Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2380743003:
Refactor ContentViewClient (2/6) (Closed)
|
Created:
4 years, 2 months ago by Jinsuk Kim Modified:
4 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, yusukes+watch_chromium.org, Peter Beverloo, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, agrieve+watch_chromium.org, James Su, android-webview-reviews_chromium.org, jochen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ContentViewClient (2/6)
Moves method |onUpdateTitle| to WebContentsObserver
See the description of https://crrev.com/2353063005 as well.
BUG=620172
Committed: https://crrev.com/c0be5c33de65d51831e2b5b2c8eda65f589bf939
Cr-Commit-Position: refs/heads/master@{#434396}
Patch Set 1 : tests #
Total comments: 9
Patch Set 2 : onUpdateTitle #Patch Set 3 : WebContentsObserver #
Total comments: 2
Patch Set 4 : // Do nothing #Messages
Total messages: 65 (34 generated)
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org, dtrainor@chromium.org, nyquist@chromium.org
nyquist@chromium.org: Please review changes in chrome/ boliu@chromium.org: Please review changes in content/, android_webview/ dtrainor@chromium.org: Please review changes in ui/android
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Haven't looked at actual CL yet, but I studied a bit how all this ActionMode stuff works. So this is a summary for all the reviewers. Right now content contains code that both populates the ActionMode menus, as well as responding to the particular menu items. And these ContentViewClient APIs allows embedder to get notification, and overwrite behavior onContextualActionBarShown/Hidden: action mode shown/hidden notification, chrome-only doesPerformWebSearch/performWebSearch: allow custom behavior for when the WebSearch action is clicked, chrome-only doesPerformProcessText/startProcessTextIntent: allow custom behavior process text action(s), these include "Translate" and "Assist", webview-only isSelectActionModeAllowed: allows embedder to remove items like share, search, etc, webview-only onStartContentIntent: called firing intents for clicking on phone number, email, address. webview-only The biggest implication of this move imo is that these callbacks are moving from a per-tab/webview interface, to a per-activity interface (WindowAndroid).
Ok so moving to per-Activity interface is not actually ok in some cases. And certainly the current CL is very broken. In siever's doc, these callbacks are supposed to go through ViewAndroid first. Then ViewAndroid can choose to forward the call to WindowAndroid if required. It's certainly better to have the call on WindowAndroid if possible but I don't think that's the case. Can live on WindowAndroid: onContextualActionBarShown/Hidden doesPerformProcessText/startProcessTextIntent Cannot live on WindowAndroid: doesPerformWebSearch/performWebSearch: Chrome needs to decide whether to start an incognito tab or not, and apparently need the reference to the parent tab. isSelectActionModeAllowed: the setting is per-webview onStartContentIntent: goes to WebViewClient which is per-webview https://codereview.chromium.org/2380743003/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2380743003/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1040: private static WindowAndroidWrapper getWindowAndroid(AwContents contents) { this doesn't work Android is by design not per-AwContents. You can't then it refer back to only the AwContents instance that started. https://codereview.chromium.org/2380743003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2380743003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1580: mWindowAndroid.setActionDelegate(new WindowAndroid.ActionDelegate() { ditto, you are overriding the old delegate with this new one https://codereview.chromium.org/2380743003/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2380743003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1902: if (getWindowAndroid().doesPerformProcessText()) { getWindowAndroid does jni, so probably better to save a local reference than repeating this call https://codereview.chromium.org/2380743003/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1967: getWindowAndroid().onContextualActionBarHidden(); getWindowAndroid can return null, but only during tab re-parenting, so it's probably fine to skip null check if user has to interact with the tab or the action mode menu looking at the calls here, this is the only one I'm not entirely sure of, so maybe add a null check here? https://codereview.chromium.org/2380743003/diff/60001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/2380743003/diff/60001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:55: protected TestCallbackHelperContainer(TestWebContentsObserver contentsObserver) { anything actually subclassing this and calling this constructor? https://codereview.chromium.org/2380743003/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2380743003/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:757: private ActionDelegate mActionDelegate = new ActionDelegate(); do the assignment in the constructor, also maybe move the declaration to where all other members are declared?
So there's a few way to salvage this: option 1) pass a ViewAndroid for calls that need it option 2) refactor action mode: only have a onStartActionMode, then let embedder construct all the menus and actions. I think will only need two APIs, startActionMode and hideActionMode. Share common code in a component or something. onStartContentIntent still needs to go somewhere though Obviously option 2 is way more involved, but probably better For option 1, I still like to see the calls go through ViewAndroid rather than WindowAndroid if possible. Maybe ViewAndroidDelegate?
Description was changed from ========== Refactor ContentViewClient (2/6) Moves following methods to WindowAndroid: - onContextualActionBarShown - onContextualActionBarHidden - performWebSearch - doesPerformWebSearch - doesPerformProcessText - startProcessTextIntent - isSelectActionModeAllowed - onStartContentIntent See the description of https://crrev.com/2353063005 as well. BUG=620172 ========== to ========== Refactor ContentViewClient (2/6) Moves following methods to WindowAndroid: - onContextualActionBarShown - onContextualActionBarHidden - performWebSearch - doesPerformWebSearch - doesPerformProcessText - startProcessTextIntent - isSelectActionModeAllowed - onStartContentIntent See the description of https://crrev.com/2353063005 as well. BUG=620172 ==========
On 2016/09/30 22:47:16, boliu wrote: > So there's a few way to salvage this: > option 1) pass a ViewAndroid for calls that need it > option 2) refactor action mode: only have a onStartActionMode, then let embedder > construct all the menus and actions. I think will only need two APIs, > startActionMode and hideActionMode. Share common code in a component or > something. onStartContentIntent still needs to go somewhere though > > Obviously option 2 is way more involved, but probably better > > For option 1, I still like to see the calls go through ViewAndroid rather than > WindowAndroid if possible. Maybe ViewAndroidDelegate? So it seems crbug.com/623783 needs to be handled first. Pedro, is there anything in progress about it? I'd like to avoid duplicated work.
jinsuk: If this ends up being put on hold, could you please ping me again when this becomes relevant?
On 2016/10/04 06:19:09, nyquist wrote: > jinsuk: If this ends up being put on hold, could you please ping me again when > this becomes relevant? Sure, will do. Thanks.
> So it seems crbug.com/623783 needs to be handled first. > Pedro, is there anything in progress about it? I'd like to avoid > duplicated work. I'm currently working on refactoring action modes to be created more like desktop context menus. The goal of the refactoring is to have Blink trigger the action mode by sending a createActionMode IPC with contextmenu params (i.e. editablity, insertion/selection, etc.) so that the browser has enough information to know which items to show in the mode. I wasn't planning on changing how the special options like web search or process text work, but I haven't gotten far enough to know if I will have to. I have two CLs that I plan on landing soon that are part of my refactor: crrev.com/2390633003 and crrev.com/2201853002
On 2016/10/04 19:01:17, amaralp wrote: > > So it seems crbug.com/623783 needs to be handled first. > > Pedro, is there anything in progress about it? I'd like to avoid > > duplicated work. > > I'm currently working on refactoring action modes to be created more like > desktop context menus. The goal of the refactoring is to have Blink trigger > the action mode by sending a createActionMode IPC with contextmenu params > (i.e. editablity, insertion/selection, etc.) so that the browser has enough > information to know which items to show in the mode. I wasn't planning on > changing how the special options like web search or process text work, > but I haven't gotten far enough to know if I will have to. I have two CLs > that I plan on landing soon that are part of my refactor: > crrev.com/2390633003 and crrev.com/2201853002 Thanks for the info. I didn't mean to say you need to work on the changes in this CL - just wanted to know if the refactoring on action mode is going on and how/if it will affect what's being done here. Will take a look at your CLs to act accordingly.
> Thanks for the info. I didn't mean to say you need to work on the changes > in this CL - just wanted to know if the refactoring on action mode is > going on and how/if it will affect what's being done here. Will take a look > at your CLs to act accordingly. Pedro's CLs are orthogonal to the refactoring in this CL. So I think this CL can continue. I'll be OOO for the rest of the week. Will go with option 2 that Bo suggested as it's also a part of the overall refactoring plan crbug.com/623783.
On 2016/10/05 07:44:24, Jinsuk_OOO (- Oct 10) wrote: > > Thanks for the info. I didn't mean to say you need to work on the changes > > in this CL - just wanted to know if the refactoring on action mode is > > going on and how/if it will affect what's being done here. Will take a look > > at your CLs to act accordingly. > > Pedro's CLs are orthogonal to the refactoring in this CL. So I think this > CL can continue. > > I'll be OOO for the rest of the week. Will go with option 2 that Bo > suggested as it's also a part of the overall refactoring plan crbug.com/623783. I didn't get a good sense what sievers actually wanted from crbug.com/623783. So if you have a better idea, or when you get a better idea, probably good to summarize it a bit on the bug first :)
nits on WindowAndroid.java, but ping me if you want/are ready for a full review. Thanks! https://codereview.chromium.org/2380743003/diff/60001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/2380743003/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:739: public static void startActivityForIntentUrl(Context context, String intentUrl) { javadoc? https://codereview.chromium.org/2380743003/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:759: public void setActionDelegate(ActionDelegate delegate) { Simple javadoc https://codereview.chromium.org/2380743003/diff/60001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:763: public void onContextualActionBarShown() { Should we have javadocs on these? I'd be fine with @see ... to refer to the delegate
On 2016/10/05 15:33:18, boliu wrote: > On 2016/10/05 07:44:24, Jinsuk_OOO (- Oct 10) wrote: > > > Thanks for the info. I didn't mean to say you need to work on the changes > > > in this CL - just wanted to know if the refactoring on action mode is > > > going on and how/if it will affect what's being done here. Will take a look > > > at your CLs to act accordingly. > > > > Pedro's CLs are orthogonal to the refactoring in this CL. So I think this > > CL can continue. > > > > I'll be OOO for the rest of the week. Will go with option 2 that Bo > > suggested as it's also a part of the overall refactoring plan > crbug.com/623783. > > I didn't get a good sense what sievers actually wanted from crbug.com/623783. So > if you have a better idea, or when you get a better idea, probably good to > summarize it a bit on the bug first :) Basically I'm thinking of embedder provide WebActionModeCallback.ActionHandler impl rather than CVC define a (mostly) common one and let it delegate further to ContentViewClient per impl-specific differences. I'll make a summary after a bit of an experiment to test its feasibility.
On 2016/10/10 23:10:52, Jinsuk Kim wrote: > On 2016/10/05 15:33:18, boliu wrote: > > On 2016/10/05 07:44:24, Jinsuk_OOO (- Oct 10) wrote: > > > > Thanks for the info. I didn't mean to say you need to work on the changes > > > > in this CL - just wanted to know if the refactoring on action mode is > > > > going on and how/if it will affect what's being done here. Will take a > look > > > > at your CLs to act accordingly. > > > > > > Pedro's CLs are orthogonal to the refactoring in this CL. So I think this > > > CL can continue. > > > > > > I'll be OOO for the rest of the week. Will go with option 2 that Bo > > > suggested as it's also a part of the overall refactoring plan > > crbug.com/623783. > > > > I didn't get a good sense what sievers actually wanted from crbug.com/623783. > So > > if you have a better idea, or when you get a better idea, probably good to > > summarize it a bit on the bug first :) > > Basically I'm thinking of embedder provide WebActionModeCallback.ActionHandler > impl rather than CVC define a (mostly) common one and let it delegate > further to ContentViewClient per impl-specific differences. I'll make a summary > after a bit of an experiment to test its feasibility. I'd like to do https://crbug.com/623783 before this CL. The scope of work for this CL will be smaller once it is take care of. https://crrev.com/2407303005
On 2016/10/14 11:38:55, Jinsuk Kim wrote: > On 2016/10/10 23:10:52, Jinsuk Kim wrote: > > On 2016/10/05 15:33:18, boliu wrote: > > > On 2016/10/05 07:44:24, Jinsuk_OOO (- Oct 10) wrote: > > > > > Thanks for the info. I didn't mean to say you need to work on the > changes > > > > > in this CL - just wanted to know if the refactoring on action mode is > > > > > going on and how/if it will affect what's being done here. Will take a > > look > > > > > at your CLs to act accordingly. > > > > > > > > Pedro's CLs are orthogonal to the refactoring in this CL. So I think this > > > > CL can continue. > > > > > > > > I'll be OOO for the rest of the week. Will go with option 2 that Bo > > > > suggested as it's also a part of the overall refactoring plan > > > crbug.com/623783. > > > > > > I didn't get a good sense what sievers actually wanted from > crbug.com/623783. > > So > > > if you have a better idea, or when you get a better idea, probably good to > > > summarize it a bit on the bug first :) > > > > Basically I'm thinking of embedder provide WebActionModeCallback.ActionHandler > > > impl rather than CVC define a (mostly) common one and let it delegate > > further to ContentViewClient per impl-specific differences. I'll make a > summary > > after a bit of an experiment to test its feasibility. > > I'd like to do https://crbug.com/623783 before this CL. The scope of work > for this CL will be smaller once it is take care of. > https://crrev.com/2407303005 https://crrev.com/2407303005 covered everything except onStartContentIntent(). The interface which I think appropriate for it would be WebContentsDelegate(Android) since it has to do with web content but not related with view. Maybe ContentViewClient.onUpdateTitle as well. wdyt?
On 2016/11/10 18:41:04, Jinsuk Kim wrote: > On 2016/10/14 11:38:55, Jinsuk Kim wrote: > > On 2016/10/10 23:10:52, Jinsuk Kim wrote: > > > On 2016/10/05 15:33:18, boliu wrote: > > > > On 2016/10/05 07:44:24, Jinsuk_OOO (- Oct 10) wrote: > > > > > > Thanks for the info. I didn't mean to say you need to work on the > > changes > > > > > > in this CL - just wanted to know if the refactoring on action mode is > > > > > > going on and how/if it will affect what's being done here. Will take a > > > look > > > > > > at your CLs to act accordingly. > > > > > > > > > > Pedro's CLs are orthogonal to the refactoring in this CL. So I think > this > > > > > CL can continue. > > > > > > > > > > I'll be OOO for the rest of the week. Will go with option 2 that Bo > > > > > suggested as it's also a part of the overall refactoring plan > > > > crbug.com/623783. > > > > > > > > I didn't get a good sense what sievers actually wanted from > > crbug.com/623783. > > > So > > > > if you have a better idea, or when you get a better idea, probably good to > > > > summarize it a bit on the bug first :) > > > > > > Basically I'm thinking of embedder provide > WebActionModeCallback.ActionHandler > > > > > impl rather than CVC define a (mostly) common one and let it delegate > > > further to ContentViewClient per impl-specific differences. I'll make a > > summary > > > after a bit of an experiment to test its feasibility. > > > > I'd like to do https://crbug.com/623783 before this CL. The scope of work > > for this CL will be smaller once it is take care of. > > https://crrev.com/2407303005 > > https://crrev.com/2407303005 covered everything except onStartContentIntent(). > > The interface which I think appropriate for it would be > WebContentsDelegate(Android) since it has to do with web content but not related > with view. Maybe ContentViewClient.onUpdateTitle as well. wdyt? Maybe leave it there for now. We intend to remove it entirely from chrome, but webview may (or may not) still need to support it. But at least the current plan is to disable content detection, ship to stable, and see who complains: https://codereview.chromium.org/2496763002/
Description was changed from ========== Refactor ContentViewClient (2/6) Moves following methods to WindowAndroid: - onContextualActionBarShown - onContextualActionBarHidden - performWebSearch - doesPerformWebSearch - doesPerformProcessText - startProcessTextIntent - isSelectActionModeAllowed - onStartContentIntent See the description of https://crrev.com/2353063005 as well. BUG=620172 ========== to ========== Refactor ContentViewClient (2/6) Moves method |onUpdateTitle| to ViewAndroid. See the description of https://crrev.com/2353063005 as well. BUG=620172 ==========
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Now the scope of patch is diminished to moving onUpdateTitle only. Ended up handling it ViewAndroid, after the way aura does the job with aura::Window::SetTitle(). PTAL.
the native one lives on WebContentsObserver: https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... so I think this can move to the java one
On 2016/11/22 04:38:18, boliu wrote: > the native one lives on WebContentsObserver: > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... > > so I think this can move to the java one Do you mean 'using WebContentsObserver/Proxy to forward to Java layer' from observer.TitleWasSet in WebContentsImpl::UpdateTitleForEntry? I think WebContentsObserver::TitleWasSet is supposed to be used for sending notification after a title is set, rather than to do job of setting the title itself. And |TitleWasSet| does not always carry the title to set (i.e. when |entry| is nullptr).
On 2016/11/22 05:11:13, Jinsuk Kim wrote: > On 2016/11/22 04:38:18, boliu wrote: > > the native one lives on WebContentsObserver: > > > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... > > > > so I think this can move to the java one > > Do you mean 'using WebContentsObserver/Proxy to forward to Java layer' from > observer.TitleWasSet in WebContentsImpl::UpdateTitleForEntry? > > I think WebContentsObserver::TitleWasSet is supposed to be used for sending > notification after a title is set, rather than to do job of setting the title > itself. And |TitleWasSet| does not always carry the title to set (i.e. when > |entry| is nullptr). You can call WebContents::GetTitle to get the title
> You can call WebContents::GetTitle to get the title That's good to know. But you didn't make a comment on the following point: > I think WebContentsObserver::TitleWasSet is supposed to be used for sending > notification after a title is set, rather than to do job of setting the title > itself.
On 2016/11/22 06:21:15, Jinsuk Kim wrote: > > You can call WebContents::GetTitle to get the title > > That's good to know. But you didn't make a comment on the following point: > > > I think WebContentsObserver::TitleWasSet is supposed to be used for sending > > notification after a title is set, rather than to do job of setting the title > > itself. I don't understand what you mean there. What do you mean "job of setting the title itself"? Do you mean setting the title on WebContents, or on the tab? Why can't tab just observer its WebContents then? The CVC implementation is called at the same place, so there would not be any change in behavior. imo there is no need for content to go through the ViewAndroid tree to talk to content embedder.
> I don't understand what you mean there. What do you mean "job of setting the > title itself"? Do you mean setting the title on WebContents, or on the tab? Why > can't tab just observer its WebContents then? The CVC implementation is called > at the same place, so there would not be any change in behavior. > > imo there is no need for content to go through the ViewAndroid tree to talk to > content embedder. Fair enough - makes sense to me now. PTAL.
Description was changed from ========== Refactor ContentViewClient (2/6) Moves method |onUpdateTitle| to ViewAndroid. See the description of https://crrev.com/2353063005 as well. BUG=620172 ========== to ========== Refactor ContentViewClient (2/6) Moves method |onUpdateTitle| to WebContentsObserver See the description of https://crrev.com/2353063005 as well. BUG=620172 ==========
lgtm % one comment https://codereview.chromium.org/2380743003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2380743003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:148: NOTIMPLEMENTED(); it's not "not implemented", there's nothing to implement, just add a comment and leave it no-op
nyquist@ would you review and change in chrome/? https://codereview.chromium.org/2380743003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_view_android.cc (right): https://codereview.chromium.org/2380743003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_view_android.cc:148: NOTIMPLEMENTED(); On 2016/11/23 03:14:11, boliu wrote: > it's not "not implemented", there's nothing to implement, just add a comment and > leave it no-op Done.
//chrome lgtm
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2380743003/#ps120001 (title: "// Do nothing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by jinsukkim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480023961473490,
"parent_rev": "bc1c1f04833798b44c23a37b91e8a77e2e8b3b26", "commit_rev":
"0b3406e20afcbf3244d0766d05ca1a272efd9abe"}
Message was sent while issue was closed.
Description was changed from ========== Refactor ContentViewClient (2/6) Moves method |onUpdateTitle| to WebContentsObserver See the description of https://crrev.com/2353063005 as well. BUG=620172 ========== to ========== Refactor ContentViewClient (2/6) Moves method |onUpdateTitle| to WebContentsObserver See the description of https://crrev.com/2353063005 as well. BUG=620172 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Refactor ContentViewClient (2/6) Moves method |onUpdateTitle| to WebContentsObserver See the description of https://crrev.com/2353063005 as well. BUG=620172 ========== to ========== Refactor ContentViewClient (2/6) Moves method |onUpdateTitle| to WebContentsObserver See the description of https://crrev.com/2353063005 as well. BUG=620172 Committed: https://crrev.com/c0be5c33de65d51831e2b5b2c8eda65f589bf939 Cr-Commit-Position: refs/heads/master@{#434396} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c0be5c33de65d51831e2b5b2c8eda65f589bf939 Cr-Commit-Position: refs/heads/master@{#434396} |
