New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 787989 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Dec 2017
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Feature

Blocking:
issue 779857



Sign in to add a comment

extensions: Refactor code to install and open bookmark apps into a utils namespace

Project Member Reported by ortuno@chromium.org, Nov 22 2017

Issue description

The ExtensionBrowserTest suite has InstallBookmarkApp() and LaunchAppBrowser(). We re-implement these two methods in render_view_context_menu_browsertest.cc. Instead of reimplementing these methods we should put the methods inside a utils file and change ExtensionBrowserTest and RenderViewContextMenuBrowserTest to use the utils file. In the future, SSLBrowserTest will also use these methods.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd2919bdcacdeaa313d92157875de30181c875eb

commit cd2919bdcacdeaa313d92157875de30181c875eb
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Thu Nov 23 10:11:53 2017

extensions: Add methods to install and open apps to browsertest_util

Refactors code to install and open apps out of ExtensionBrowserTest and
into browsertest_util. So that other test suites that need to test Bookmark
Apps e.g. RenderViewContextMenuBrowserTest and in the future SSLBrowserTest
can use them.

Bug:  787989 
Change-Id: Ic8436e07c888c72d286352345f5ec134c3e3a640
Reviewed-on: https://chromium-review.googlesource.com/786690
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518882}
[modify] https://crrev.com/cd2919bdcacdeaa313d92157875de30181c875eb/chrome/browser/extensions/browsertest_util.cc
[modify] https://crrev.com/cd2919bdcacdeaa313d92157875de30181c875eb/chrome/browser/extensions/browsertest_util.h
[modify] https://crrev.com/cd2919bdcacdeaa313d92157875de30181c875eb/chrome/browser/extensions/extension_browsertest.cc
[modify] https://crrev.com/cd2919bdcacdeaa313d92157875de30181c875eb/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/126ddb6ca14b41101743d78142968b4d41469dc1

commit 126ddb6ca14b41101743d78142968b4d41469dc1
Author: Olga Sharonova <olka@chromium.org>
Date: Thu Nov 23 12:31:10 2017

Revert "extensions: Add methods to install and open apps to browsertest_util"

This reverts commit cd2919bdcacdeaa313d92157875de30181c875eb.

Reason for revert:  Bug 788156  (NativeBindings/MessagingApiTest.Messaging/0 failing on linux-chromeos-rel)

Original change's description:
> extensions: Add methods to install and open apps to browsertest_util
> 
> Refactors code to install and open apps out of ExtensionBrowserTest and
> into browsertest_util. So that other test suites that need to test Bookmark
> Apps e.g. RenderViewContextMenuBrowserTest and in the future SSLBrowserTest
> can use them.
> 
> Bug:  787989 
> Change-Id: Ic8436e07c888c72d286352345f5ec134c3e3a640
> Reviewed-on: https://chromium-review.googlesource.com/786690
> Reviewed-by: Matt Giuca <mgiuca@chromium.org>
> Reviewed-by: Ben Wells <benwells@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#518882}

TBR=benwells@chromium.org,mgiuca@chromium.org,ortuno@chromium.org,jochen@chromium.org

Change-Id: I4f70853c4e8171db92d6a46b0995b7d31d87ee6f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  787989 
Reviewed-on: https://chromium-review.googlesource.com/787750
Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518906}
[modify] https://crrev.com/126ddb6ca14b41101743d78142968b4d41469dc1/chrome/browser/extensions/browsertest_util.cc
[modify] https://crrev.com/126ddb6ca14b41101743d78142968b4d41469dc1/chrome/browser/extensions/browsertest_util.h
[modify] https://crrev.com/126ddb6ca14b41101743d78142968b4d41469dc1/chrome/browser/extensions/extension_browsertest.cc
[modify] https://crrev.com/126ddb6ca14b41101743d78142968b4d41469dc1/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa21df7ce9326fd8df1c67de62836336e8a68504

commit fa21df7ce9326fd8df1c67de62836336e8a68504
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Fri Nov 24 01:03:49 2017

Reland "extensions: Add methods to install and open apps to browsertest_util"

The patch was reverted because NativeBindings/MessagingApiTest.Messaging/0
started failing:

https://luci-milo.appspot.com/buildbot/chromium.chromiumos/linux-chromeos-rel/2871

There have been other instances of this test failing in the past
which indicates the test might be flaky. Furthermore, the changes in
this patch don't affect the test in question. All MessagingApiTests pass
when adding DCHECK(false) to the newly added functions.

This is a reland of cd2919bdcacdeaa313d92157875de30181c875eb
Original change's description:
> extensions: Add methods to install and open apps to browsertest_util
>
> Refactors code to install and open apps out of ExtensionBrowserTest and
> into browsertest_util. So that other test suites that need to test Bookmark
> Apps e.g. RenderViewContextMenuBrowserTest and in the future SSLBrowserTest
> can use them.
>
> Bug:  787989 
> Change-Id: Ic8436e07c888c72d286352345f5ec134c3e3a640
> Reviewed-on: https://chromium-review.googlesource.com/786690
> Reviewed-by: Matt Giuca <mgiuca@chromium.org>
> Reviewed-by: Ben Wells <benwells@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#518882}

TBR=jochen@chromium.org, benwells@chromium.org,

Bug:  787989 
Change-Id: I464611dc94511e6c0589c3aaef91f1affdc0aab1
Reviewed-on: https://chromium-review.googlesource.com/788550
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519037}
[modify] https://crrev.com/fa21df7ce9326fd8df1c67de62836336e8a68504/chrome/browser/extensions/browsertest_util.cc
[modify] https://crrev.com/fa21df7ce9326fd8df1c67de62836336e8a68504/chrome/browser/extensions/browsertest_util.h
[modify] https://crrev.com/fa21df7ce9326fd8df1c67de62836336e8a68504/chrome/browser/extensions/extension_browsertest.cc
[modify] https://crrev.com/fa21df7ce9326fd8df1c67de62836336e8a68504/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc

Status: Fixed (was: Started)

Sign in to add a comment