Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(23)

Issue 2943983003: chrome/blink: Add functionality for in-product help for media elements. (Closed)

Created:
3 years, 6 months ago by Khushal
Modified:
3 years, 4 months ago
CC:
Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, haraken, jam, kinuko+watch, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, platform-architecture-syd+reviews-web_chromium.org, qsr+mojo_chromium.org, rwlbuis, sof, srahim+watch_chromium.org, Srirama, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

chrome/blink: Add functionality for in-product help for media elements. IPH (In-product help) UI is added to highlight and assist in the discovery of features in the browser. This change adds this functionality for the download feature on media elements in blink. This involves the following additions: 1) A public WebMediaIPH interface in blink that can be used to ask the embedder to display IPH widgets for an element. 2) A mojo MediaIPHService to bridge these requests to the browser where the UI can be shown using android pop-ups overlayed on the content. 3) An addition to the features supported by the FeatureEngagementTracker for media download IPH. The feature is currently gated behind a finch flag which is disabled by default. BUG=715185 Review-Url: https://codereview.chromium.org/2943983003 Cr-Commit-Position: refs/heads/master@{#496361} Committed: https://chromium.googlesource.com/chromium/src/+/c5447db2943848192c0452c7b8e766a943335f07

Patch Set 1 #

Total comments: 15

Patch Set 2 : addressed comments #

Total comments: 12

Patch Set 3 : addressed comments #

Total comments: 13

Patch Set 4 : rebase #

Patch Set 5 : sam's comments #

Patch Set 6 : fix dismiss on shutdown #

Patch Set 7 : nyquist comments #

Total comments: 4

Patch Set 8 : tests #

Total comments: 2

Patch Set 9 : move to button #

Total comments: 5

Patch Set 10 : dtrainor@'s comments. #

Patch Set 11 : not on pause. #

Total comments: 21

Patch Set 12 : rebase #

Patch Set 13 : mounir's comments. #

Total comments: 11

Patch Set 14 : rebase #

Patch Set 15 : some comments addressed #

Patch Set 16 : move IPH to MediaDownloadInProductManager #

Total comments: 26

Patch Set 17 : mounir's comments. #

Patch Set 18 : .. #

Total comments: 7

Patch Set 19 : .. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+701 lines, -10 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +50 lines, -0 lines 0 comments Download
M chrome/android/java/strings/android_chrome_strings.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +6 lines, -1 line 0 comments Download
M chrome/browser/android/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/android/tab_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 6 chunks +37 lines, -1 line 0 comments Download
M chrome/browser/android/tab_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 7 chunks +112 lines, -1 line 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_manifest_overlay.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M components/feature_engagement/public/android/java/src/org/chromium/components/feature_engagement/EventConstants.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +5 lines, -0 lines 0 comments Download
M components/feature_engagement/public/android/java/src/org/chromium/components/feature_engagement/FeatureConstants.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -0 lines 0 comments Download
M components/feature_engagement/public/feature_constants.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M components/feature_engagement/public/feature_constants.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -0 lines 0 comments Download
M components/feature_engagement/public/feature_list.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +2 lines, -0 lines 0 comments Download
M components/feature_engagement/public/feature_list.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/common_param_traits_macros.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/web_preferences.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/web_preferences.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/exported/WebSettingsImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/exported/WebSettingsImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/Settings.json5 View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/DEPS View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 10 chunks +45 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/MediaControlsImplTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +137 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +52 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +105 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlDownloadButtonElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/elements/MediaControlInputElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/mojo/Geometry.typemap View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/mojo/GeometryStructTraits.h View 1 2 2 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/mojo/GeometryStructTraits.cpp View 1 2 1 chunk +13 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/mojo/GeometryStructTraitsTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +14 lines, -3 lines 0 comments Download
M third_party/WebKit/public/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
A third_party/WebKit/public/platform/media_download_in_product_help.mojom View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +16 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebSettings.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/actions/actions.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +37 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 73 (31 generated)
Khushal
mlamouri@, could you take a look at the blink side of changes and see if ...
3 years, 6 months ago (2017-06-20 09:19:28 UTC) #2
Khushal
+dtrainor for chrome and feature_engagement_tracker.
3 years, 6 months ago (2017-06-20 09:20:55 UTC) #4
Khushal
+chrishtr
3 years, 6 months ago (2017-06-21 20:07:37 UTC) #8
Khushal
pingy?
3 years, 6 months ago (2017-06-22 20:00:54 UTC) #9
chrishtr
https://codereview.chromium.org/2943983003/diff/1/chrome/browser/android/media_iph_manager.h File chrome/browser/android/media_iph_manager.h (right): https://codereview.chromium.org/2943983003/diff/1/chrome/browser/android/media_iph_manager.h#newcode23 chrome/browser/android/media_iph_manager.h:23: virtual void UpdateMediaDownloadIPHPosition( You can remove this method and ...
3 years, 6 months ago (2017-06-23 00:39:45 UTC) #10
Khushal
https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/core/dom/MediaIPHManager.h File third_party/WebKit/Source/core/dom/MediaIPHManager.h (right): https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/core/dom/MediaIPHManager.h#newcode18 third_party/WebKit/Source/core/dom/MediaIPHManager.h:18: class CORE_EXPORT MediaIPHManager final On 2017/06/23 00:39:44, chrishtr wrote: ...
3 years, 6 months ago (2017-06-23 02:11:06 UTC) #11
Khushal
On 2017/06/23 02:11:06, Khushal wrote: > https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/core/dom/MediaIPHManager.h > File third_party/WebKit/Source/core/dom/MediaIPHManager.h (right): > > https://codereview.chromium.org/2943983003/diff/1/third_party/WebKit/Source/core/dom/MediaIPHManager.h#newcode18 > ...
3 years, 5 months ago (2017-06-27 00:05:19 UTC) #12
chrishtr
My suggestions are all targeted around trying to reduce the complexity of this patch, and ...
3 years, 5 months ago (2017-06-27 18:22:20 UTC) #13
Khushal
+haraken for the mojom bits. I have added the interface in platform/ for now, until ...
3 years, 5 months ago (2017-06-28 05:32:38 UTC) #15
haraken
(I chatted with Sam about this change. Sam has some thoughts about this.) +sammc
3 years, 5 months ago (2017-06-28 08:36:33 UTC) #21
Sam McNally
https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Source/core/frame/LocalFrame.cpp File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode1279 third_party/WebKit/Source/core/frame/LocalFrame.cpp:1279: void LocalFrame::ShowDownloadMediaInProductHelp(HTMLMediaElement& elem) { Could this be handled within ...
3 years, 5 months ago (2017-06-28 09:19:58 UTC) #22
Khushal
https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Source/core/frame/LocalFrame.cpp File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode1279 third_party/WebKit/Source/core/frame/LocalFrame.cpp:1279: void LocalFrame::ShowDownloadMediaInProductHelp(HTMLMediaElement& elem) { On 2017/06/28 09:19:58, Sam McNally ...
3 years, 5 months ago (2017-06-28 22:28:46 UTC) #23
nyquist
I mostly looked at //components/feature_engagement_tracker and //chrome, and that part looks reasonable to me. https://codereview.chromium.org/2943983003/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java ...
3 years, 5 months ago (2017-06-29 02:10:24 UTC) #24
Sam McNally
https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Source/core/frame/LocalFrame.cpp File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode1279 third_party/WebKit/Source/core/frame/LocalFrame.cpp:1279: void LocalFrame::ShowDownloadMediaInProductHelp(HTMLMediaElement& elem) { On 2017/06/28 22:28:46, Khushal wrote: ...
3 years, 5 months ago (2017-07-03 09:45:41 UTC) #25
Khushal
https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Source/core/frame/LocalFrame.cpp File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2943983003/diff/20001/third_party/WebKit/Source/core/frame/LocalFrame.cpp#newcode1279 third_party/WebKit/Source/core/frame/LocalFrame.cpp:1279: void LocalFrame::ShowDownloadMediaInProductHelp(HTMLMediaElement& elem) { On 2017/07/03 09:45:40, Sam McNally ...
3 years, 5 months ago (2017-07-07 04:18:14 UTC) #26
Sam McNally
mojom and related files LGTM. https://codereview.chromium.org/2943983003/diff/120001/chrome/browser/android/media_in_product_help_manager.cc File chrome/browser/android/media_in_product_help_manager.cc (right): https://codereview.chromium.org/2943983003/diff/120001/chrome/browser/android/media_in_product_help_manager.cc#newcode49 chrome/browser/android/media_in_product_help_manager.cc:49: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2943983003/diff/120001/chrome/browser/android/media_in_product_help_manager.cc#newcode90 chrome/browser/android/media_in_product_help_manager.cc:90: ...
3 years, 5 months ago (2017-07-07 05:22:58 UTC) #33
mlamouri (slow - plz ping)
Two high level comments: - I would prefer if we could have the service hooking ...
3 years, 5 months ago (2017-07-07 14:52:10 UTC) #36
whywhat
a drive-by nit I had a couple of weeks ago (the rest seems to have ...
3 years, 5 months ago (2017-07-10 15:29:28 UTC) #38
nyquist
On 2017/07/07 14:52:10, mlamouri (slow - plz ping) wrote: > Two high level comments: > ...
3 years, 5 months ago (2017-07-10 20:09:42 UTC) #39
Khushal
On 2017/07/10 20:09:42, nyquist (OOO until 6-10) wrote: > On 2017/07/07 14:52:10, mlamouri (slow - ...
3 years, 5 months ago (2017-07-10 20:35:41 UTC) #40
Khushal
https://codereview.chromium.org/2943983003/diff/140001/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2943983003/diff/140001/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp#newcode1230 third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:1230: void MediaControlsImpl::MaybeDispatchDownloadIPHTrigger() { On 2017/07/10 15:29:28, whywhat wrote: > ...
3 years, 5 months ago (2017-07-10 20:38:02 UTC) #41
David Trainor- moved to gerrit
lgtm! https://codereview.chromium.org/2943983003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2943983003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode3144 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3144: mDownloadIPHBubble.setAnchorRect(rect); Does it make sense to do this ...
3 years, 5 months ago (2017-07-11 16:34:42 UTC) #42
Khushal
Thanks Dave. https://codereview.chromium.org/2943983003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2943983003/diff/160001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode3144 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3144: mDownloadIPHBubble.setAnchorRect(rect); On 2017/07/11 16:34:42, David Trainor-ping after ...
3 years, 5 months ago (2017-07-11 20:34:36 UTC) #43
nyquist
I looked at //chrome, //components and //content, and it lgtm. I also looked at the ...
3 years, 5 months ago (2017-07-11 22:49:19 UTC) #44
Khushal
Thanks nyquist. ping to mlamouri@ for media_controls/.
3 years, 5 months ago (2017-07-11 23:04:24 UTC) #45
mlamouri (slow - plz ping)
See some comments below. Would it be possible to add unit test for this in ...
3 years, 5 months ago (2017-07-13 21:14:27 UTC) #46
Khushal
Thanks Mounir. I added a few tests in MediaControlsImplTest. Let me know if there is ...
3 years, 5 months ago (2017-07-17 17:52:21 UTC) #47
Khushal
+jochen for chrome/, content/, third_party/Webkit. +dcheng for mojo IPC review. pingy to mlamouri@ for media_controls. ...
3 years, 5 months ago (2017-07-19 23:48:50 UTC) #49
Ilya Sherman
metrics lgtm
3 years, 5 months ago (2017-07-20 00:05:19 UTC) #50
jochen (gone - plz use gerrit)
I reviewed content/, chrome/ (except for files in android/ directories), and WebKit/Source/core and public/ those ...
3 years, 5 months ago (2017-07-24 10:53:10 UTC) #52
mlamouri (slow - plz ping)
https://codereview.chromium.org/2943983003/diff/240001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2943983003/diff/240001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode3129 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3129: if (tracker.shouldTriggerHelpUI(FeatureConstants.MEDIA_DOWNLOAD_FEATURE)) { Maybe you could do: ``` if ...
3 years, 5 months ago (2017-07-25 17:29:09 UTC) #53
Khushal
https://codereview.chromium.org/2943983003/diff/240001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2943983003/diff/240001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode3129 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3129: if (tracker.shouldTriggerHelpUI(FeatureConstants.MEDIA_DOWNLOAD_FEATURE)) { On 2017/07/25 17:29:08, mlamouri (slow - ...
3 years, 4 months ago (2017-08-03 02:41:22 UTC) #54
Khushal
I've moved all of the state tracking to a seperate MediaDownloadInProductHelpManager with some methods on ...
3 years, 4 months ago (2017-08-16 04:10:30 UTC) #55
mlamouri (slow - plz ping)
lgtm with comments applied. I was also wondering what would happen if the user were ...
3 years, 4 months ago (2017-08-16 13:09:31 UTC) #56
Khushal
Thanks mlamouri! +dcheng for IPC review. https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp File third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp (right): https://codereview.chromium.org/2943983003/diff/300001/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp#newcode301 third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp:301: CreateDownloadInProductHelp(); On 2017/08/16 ...
3 years, 4 months ago (2017-08-16 19:01:23 UTC) #58
Khushal
https://codereview.chromium.org/2943983003/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2943983003/diff/340001/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java#newcode3131 chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:3131: mDownloadIPHBubble.setDismissOnTouchInteraction(true); Oh and about the user scrolling/interacting with the ...
3 years, 4 months ago (2017-08-16 21:02:06 UTC) #59
dcheng
https://codereview.chromium.org/2943983003/diff/340001/chrome/browser/android/tab_android.cc File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2943983003/diff/340001/chrome/browser/android/tab_android.cc#newcode137 chrome/browser/android/tab_android.cc:137: content::RenderFrameHost* render_frame_host_; Nit: content::RenderFrameHost* const https://codereview.chromium.org/2943983003/diff/340001/third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.h File third_party/WebKit/Source/modules/media_controls/MediaDownloadInProductHelpManager.h (right): ...
3 years, 4 months ago (2017-08-19 00:30:15 UTC) #60
Khushal
Thanks Daniel. https://codereview.chromium.org/2943983003/diff/340001/chrome/browser/android/tab_android.cc File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/2943983003/diff/340001/chrome/browser/android/tab_android.cc#newcode137 chrome/browser/android/tab_android.cc:137: content::RenderFrameHost* render_frame_host_; On 2017/08/19 00:30:15, dcheng wrote: ...
3 years, 4 months ago (2017-08-21 23:27:34 UTC) #61
dcheng
lgtm (for future CLs, it's helpful to upload separate patchsets for rebasing and for responding ...
3 years, 4 months ago (2017-08-22 08:01:47 UTC) #66
Khushal
On 2017/08/22 08:01:47, dcheng wrote: > lgtm > > (for future CLs, it's helpful to ...
3 years, 4 months ago (2017-08-22 17:44:06 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2943983003/360001
3 years, 4 months ago (2017-08-22 17:44:30 UTC) #70
commit-bot: I haz the power
3 years, 4 months ago (2017-08-22 17:53:35 UTC) #73
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/c5447db2943848192c0452c7b8e7...

Powered by Google App Engine
This is Rietveld 408576698