Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1956633002:
Menu hook plumbed through to RequestCoordinator. (Closed)
|
Created:
4 years, 7 months ago by Pete Williamson Modified:
4 years, 7 months ago CC:
chromium-reviews, fgorski+watch_chromium.org, romax+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMenu hook plumbed through to RequestCoordinator.
We added a menu hook previously when our Background Offline Pages flag
is turned on. This changelist catches the click on the menu item,
and creates a RequestCoordinator to handle the request. We also added
a unit test for the existing RequestCoordinator factory.
Since the BrowserContext is not available from the context of the
Menu Populator or the Menu helper class, I added a method to the
delegate interface to be able to get it from the tab class (actually,
I create the OfflinePageBridge in the tab delegate so we don't have
to pass the browser context around just to get the OfflinePageBridge.
BUG=
Committed: https://crrev.com/d3849706b141072fcca07c728cd19af07ad3a56b
Cr-Commit-Position: refs/heads/master@{#392391}
Patch Set 1 #
Total comments: 12
Patch Set 2 : CR feedback per FGorski && DougArnett #
Total comments: 14
Patch Set 3 : CR fixes per JianLi #Patch Set 4 : Improve namespace name. #
Total comments: 4
Patch Set 5 : A few more changes per JianLi and FGorski. #
Total comments: 4
Patch Set 6 : CR feedback per BauerB #Patch Set 7 : Remove reference to unit test which I forgot to include #Messages
Total messages: 30 (10 generated)
petewil@chromium.org changed reviewers: + dougarnett@chromium.org, jianli@chromium.org
Jian: Please review the changes to the OfflinePageBridge, both C++ and Java sides. Doug: Please review the changes to RequestCoordinator related code, and double check all the menu related code. Once we are happy with everything else, I'll add a reviewer for the ChromeContextMenuPopupator, helper, and delegate.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemDelegate.java (right): https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemDelegate.java:117: OfflinePageBridge getOfflinePageBridge(); Looking at how this interface is structured, I think you should have an action here: savePageLater(url). or: onSavePageLater(url) Implementation should be on: TabContextMenuItemDelegate You don't have to change ContextMenuHelper. https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_bridge.cc:334: offline_pages::SavePageRequest request( Pete, use the same signature as for SavePage: url, client_id, + maybe some data later. Request is an internal thing and will be created/assigned a proper ID, by request coordinator (or maybe even by the queue for the later. https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:48: // TODO(petewil): Implement. remove todo? Also, you probably want to make sure incognito is handled properly.
https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:353: } else if (itemId == R.id.contextmenu_save_offline) { btw, I wonder if we should use the save_page_later terminology for the menu option instead of save_offline in order to tie it more directly to the api terminology https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:131: ClientId clientId = new ClientId("LinkLoader", Long.toString(offline_id)); Maybe "AsyncLoader" better namespace to start with (TODO - define string someplace better) https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:48: // TODO(petewil): Implement. On 2016/05/06 07:03:09, fgorski wrote: > remove todo? > > Also, you probably want to make sure incognito is handled properly. Right, maybe just adjust TODO wording about supporting incognito
CR feedback for FGorski && DougArnett https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:353: } else if (itemId == R.id.contextmenu_save_offline) { On 2016/05/06 16:47:41, dougarnett wrote: > btw, I wonder if we should use the save_page_later terminology for the menu > option instead of save_offline in order to tie it more directly to the api > terminology Left as is for now. Remember that this is not speced UI, but just a test hook for us, and I think for us, having it say "offline the linked page" is better. If PM/UX decides they like this feature, we will ask UX for the best string to use here. https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:131: ClientId clientId = new ClientId("LinkLoader", Long.toString(offline_id)); On 2016/05/06 16:47:41, dougarnett wrote: > Maybe "AsyncLoader" better namespace to start with (TODO - define string > someplace better) Done. (You will see the change in TabContextMenuItemDelegate.java instead of here since I moved it due to another piece of CR feedback). https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemDelegate.java (right): https://codereview.chromium.org/1956633002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuItemDelegate.java:117: OfflinePageBridge getOfflinePageBridge(); On 2016/05/06 07:03:09, fgorski wrote: > Looking at how this interface is structured, I think you should have an action > here: > savePageLater(url). > > or: onSavePageLater(url) > > Implementation should be on: TabContextMenuItemDelegate > > You don't have to change ContextMenuHelper. Done. https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_bridge.cc:334: offline_pages::SavePageRequest request( On 2016/05/06 07:03:09, fgorski wrote: > Pete, use the same signature as for SavePage: > > url, client_id, + maybe some data later. > > Request is an internal thing and will be created/assigned a proper ID, by > request coordinator (or maybe even by the queue for the later. Done. I had been following the interface spec that we build, but I like this way better. https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:48: // TODO(petewil): Implement. On 2016/05/06 07:03:09, fgorski wrote: > remove todo? > > Also, you probably want to make sure incognito is handled properly. Done. https://codereview.chromium.org/1956633002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/request_coordinator_factory.cc:48: // TODO(petewil): Implement. On 2016/05/06 16:47:41, dougarnett wrote: > On 2016/05/06 07:03:09, fgorski wrote: > > remove todo? > > > > Also, you probably want to make sure incognito is handled properly. > > Right, maybe just adjust TODO wording about supporting incognito Done.
lgtm
https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:354: // TODO(petewil): This is for testing only, remove it once we add nit: avoid adding comment like this. Probably better to say: This is a temporary hook up point to save a page later. https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:420: * @param ClientId The client for the offline page, and the client's ID. nit: please update comment to Client ID for the offline page to be saved later. https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:174: long offline_id = now.getTime(); I think it is a bit risky to use the time due to small likelihood of collision resulted from DST change or manual clock update. It would be better to use random number. Also, why not letting model generate this automatically, instead of adding a bunch of code to pass it from Java to C++? How would you handle the situation that same URL is being requested to save multiple times if each time we create a different id here. https://codereview.chromium.org/1956633002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:332: // Get a background request coordinator. nit: remove this comment and next comment since the codes here are simple enough to understand without comments.
lgtm with comments and seconding comments from Jian Li. https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:420: * @param ClientId The client for the offline page, and the client's ID. also it should say: clientId https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:175: ClientId clientId = new ClientId("AsyncLoader", Long.toString(offline_id)); based on "bookmark" and "last_n" this should say "async_loader". "async_loading" sounds even better as that is what the feature is about.
https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ChromeContextMenuPopulator.java:354: // TODO(petewil): This is for testing only, remove it once we add On 2016/05/06 20:39:03, jianli wrote: > nit: avoid adding comment like this. Probably better to say: This is a temporary > hook up point to save a page later. Done. https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:420: * @param ClientId The client for the offline page, and the client's ID. On 2016/05/06 20:39:03, jianli wrote: > nit: please update comment to > Client ID for the offline page to be saved later. Done. https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:174: long offline_id = now.getTime(); On 2016/05/06 20:39:04, jianli wrote: > I think it is a bit risky to use the time due to small likelihood of collision > resulted from DST change or manual clock update. It would be better to use > random number. > > Also, why not letting model generate this automatically, instead of adding a > bunch of code to pass it from Java to C++? How would you handle the situation > that same URL is being requested to save multiple times if each time we create > a different id here. Changed it to use Random. Again, this is just a temporary shim that we will remove when we are done testing. The idea is to pass the ClientID as a whole down through the bridge layer the same way that any other Java side clients are likely to do it, since the ClientID is an API level construct. If the same URL is being requested multiple times, it is the responsibility of the client to differentiate them with a different offline_id for each one. This particular call is for the Menu item that we added when EnableBackgroundOfflining is turned on. https://codereview.chromium.org/1956633002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:332: // Get a background request coordinator. On 2016/05/06 20:39:04, jianli wrote: > nit: remove this comment and next comment since the codes here are simple enough > to understand without comments. Done.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956633002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956633002/40001
Fix and comment for FGorski https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:420: * @param ClientId The client for the offline page, and the client's ID. On 2016/05/06 21:28:10, fgorski wrote: > also it should say: clientId Fixed it to say client ID before I saw your comment, I presume that is good. https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:175: ClientId clientId = new ClientId("AsyncLoader", Long.toString(offline_id)); On 2016/05/06 21:28:10, fgorski wrote: > based on "bookmark" and "last_n" this should say "async_loader". > > "async_loading" sounds even better as that is what the feature is about. Done.
lgtm https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:174: long offline_id = now.getTime(); On 2016/05/06 21:30:28, Pete Williamson wrote: > On 2016/05/06 20:39:04, jianli wrote: > > I think it is a bit risky to use the time due to small likelihood of collision > > resulted from DST change or manual clock update. It would be better to use > > random number. > > > > Also, why not letting model generate this automatically, instead of adding a > > bunch of code to pass it from Java to C++? How would you handle the situation > > that same URL is being requested to save multiple times if each time we > create > > a different id here. > > Changed it to use Random. > > Again, this is just a temporary shim that we will remove when we are done > testing. > The idea is to pass the ClientID as a whole down through the bridge layer the > same way that any other Java side clients are likely to do it, since the > ClientID is an API level construct. > > If the same URL is being requested multiple times, it is the responsibility of > the client to differentiate them with a different offline_id for each one. This > particular call is for the Menu item that we added when > EnableBackgroundOfflining is turned on. I think we probably need to give more careful thought on this. For now it is fine. But I still have the concern that duplicate requests for saving same URL might be likely, esp when you add it in dino page, and having something to handle here will become too complicated. https://codereview.chromium.org/1956633002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1956633002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:29: DVLOG(0) << "URL is " << url << " " << __FUNCTION__; nit: avoid adding this in production code https://codereview.chromium.org/1956633002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:31: // TODO(petewil): We need a robust scheme for allocating new IDs. Is this comment conflict with ID being passed in?
https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java (right): https://codereview.chromium.org/1956633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java:174: long offline_id = now.getTime(); On 2016/05/06 21:48:05, jianli wrote: > On 2016/05/06 21:30:28, Pete Williamson wrote: > > On 2016/05/06 20:39:04, jianli wrote: > > > I think it is a bit risky to use the time due to small likelihood of > collision > > > resulted from DST change or manual clock update. It would be better to use > > > random number. > > > > > > Also, why not letting model generate this automatically, instead of adding a > > > bunch of code to pass it from Java to C++? How would you handle the > situation > > > that same URL is being requested to save multiple times if each time we > > create > > > a different id here. > > > > Changed it to use Random. > > > > Again, this is just a temporary shim that we will remove when we are done > > testing. > > The idea is to pass the ClientID as a whole down through the bridge layer the > > same way that any other Java side clients are likely to do it, since the > > ClientID is an API level construct. > > > > If the same URL is being requested multiple times, it is the responsibility of > > the client to differentiate them with a different offline_id for each one. > This > > particular call is for the Menu item that we added when > > EnableBackgroundOfflining is turned on. > > I think we probably need to give more careful thought on this. For now it is > fine. But I still have the concern that duplicate requests for saving same URL > might be likely, esp when you add it in dino page, and having something to > handle here will become too complicated. Acknowledged. When we replace this test stub with a real client, we need to think hard to make sure that IDs are properly unique and requests are not duplicated. The real client will not use TabContextMenuItemDelegate, so this code will be removed, and the real client will hook to the dinosaur page. https://codereview.chromium.org/1956633002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1956633002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:29: DVLOG(0) << "URL is " << url << " " << __FUNCTION__; On 2016/05/06 21:48:05, jianli wrote: > nit: avoid adding this in production code Done. https://codereview.chromium.org/1956633002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:31: // TODO(petewil): We need a robust scheme for allocating new IDs. On 2016/05/06 21:48:05, jianli wrote: > Is this comment conflict with ID being passed in? As it turns out, there are two IDs. The ClientID is a pair of <namespace, id-unique-within-that-namespace>. There is also a request ID. The client (we don't have a real one yet, just a test hook) is responsible for making sure it's requests have unique ids. The RequestCoordinator is responsible for making sure that every request has a unique requestID, which is what this part of the code is doing. What I have now is not very sophisticated, I hope to improve this before we are done (hence the TODO).
petewil@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in ChromeContextMenuPopulator.java, ChromeContextMenuItemDelegate.java, and TabContextMenuItemDelegate.java The intent of this change is to catch a press on our menu item for saving an offline copy of a page, and start the ball rolling by forwarding the call across the JNI boundary to the C++ code responsible for starting an async background load once the network returns. Relevant design docs: go/offline-pages-design go/chrome-background-offlining Team docs: go/paquete
LGTM https://codereview.chromium.org/1956633002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1956633002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:329: client_id.name_space = ConvertJavaStringToUTF8(env, j_client_id_namespace); Nit: I would rename the field to just |namespace| without the underscore, to be consistent with other places (and save headaches later). https://codereview.chromium.org/1956633002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1956633002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:36: bool SavePageLater(const GURL url, const ClientId& client_id); Pass the GURL by (const) reference as well.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, fgorski@chromium.org, jianli@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1956633002/#ps100001 (title: "CR feedback per BauerB")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956633002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956633002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
Oops, I forgot to add the RequestCoordinatorUnittest to this patch, I will include it with the next patch instead. For now I removed it from the .gypi file to make the patch consistent. https://codereview.chromium.org/1956633002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_bridge.cc (right): https://codereview.chromium.org/1956633002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_bridge.cc:329: client_id.name_space = ConvertJavaStringToUTF8(env, j_client_id_namespace); On 2016/05/09 10:00:36, Bernhard Bauer wrote: > Nit: I would rename the field to just |namespace| without the underscore, to be > consistent with other places (and save headaches later). Done. https://codereview.chromium.org/1956633002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1956633002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:36: bool SavePageLater(const GURL url, const ClientId& client_id); On 2016/05/09 10:00:36, Bernhard Bauer wrote: > Pass the GURL by (const) reference as well. Done.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dougarnett@chromium.org, jianli@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1956633002/#ps120001 (title: "Remove reference to unit test which I forgot to include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956633002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956633002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Menu hook plumbed through to RequestCoordinator. We added a menu hook previously when our Background Offline Pages flag is turned on. This changelist catches the click on the menu item, and creates a RequestCoordinator to handle the request. We also added a unit test for the existing RequestCoordinator factory. Since the BrowserContext is not available from the context of the Menu Populator or the Menu helper class, I added a method to the delegate interface to be able to get it from the tab class (actually, I create the OfflinePageBridge in the tab delegate so we don't have to pass the browser context around just to get the OfflinePageBridge. BUG= ========== to ========== Menu hook plumbed through to RequestCoordinator. We added a menu hook previously when our Background Offline Pages flag is turned on. This changelist catches the click on the menu item, and creates a RequestCoordinator to handle the request. We also added a unit test for the existing RequestCoordinator factory. Since the BrowserContext is not available from the context of the Menu Populator or the Menu helper class, I added a method to the delegate interface to be able to get it from the tab class (actually, I create the OfflinePageBridge in the tab delegate so we don't have to pass the browser context around just to get the OfflinePageBridge. BUG= Committed: https://crrev.com/d3849706b141072fcca07c728cd19af07ad3a56b Cr-Commit-Position: refs/heads/master@{#392391} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d3849706b141072fcca07c728cd19af07ad3a56b Cr-Commit-Position: refs/heads/master@{#392391} |
