Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481443002:
Implementation (cc file) for background loader contents. (Closed)
|
Created:
4 years, 1 month ago by chili Modified:
4 years, 1 month ago CC:
chromium-reviews, chili+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, jam, petewil+watch_chromium.org, dewittj+watch_chromium.org, darin-cc_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplementation (cc file) for background loader contents.
BUG=659414
Committed: https://crrev.com/2e25132b6341e45f1704e9f1ea39d14c121273fe
Cr-Commit-Position: refs/heads/master@{#433393}
Patch Set 1 #Patch Set 2 : Format and build #
Total comments: 16
Patch Set 3 : code review #Patch Set 4 : code review comments and tests #Patch Set 5 : remove unneeded dep from tests #
Total comments: 2
Patch Set 6 : code review and rebase #Patch Set 7 : rebase-update, since there seem to be unrelated iOS issues before #Patch Set 8 : only run tests on non-iOS #Patch Set 9 : rebase #
Messages
Total messages: 39 (20 generated)
chili@chromium.org changed reviewers: + dewittj@chromium.org, dimich@chromium.org, dougarnett@chromium.org, petewil@chromium.org
Not sure if this can be tested; None of the other web contents delegates have unit tests; Also cannot find a stub profile that will allow WebContents to be successfully created on it yet....
Nice! For testing, does anything in src/content/public/test work? There is a TestBrowserContext, TestWebContents etc... https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.cc (right): https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:23: url, content::Referrer(), ui::PAGE_TRANSITION_LINK, std::string()); Could you add comment on the choice of parameter values? Also, the std::string() param - need to add comment on what it is. https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:30: void BackgroundLoaderContents::Cancel() { The methods in cc file should be in the same order as they are in .h file. (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md, "Code Formatting") https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:52: callback.Run(false); It'd be nice to look at callers of CanDownload and check if they can consume the reentrant call to the callback. Often it is safer to PostTask with a callback call to make sure it comes on a clean stack. Same for other callback.Run() calls in this file.
nice! https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... File components/offline_pages/content/background_loader/BUILD.gn (right): https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/BUILD.gn:9: static_library("background_loader") { do we want this to be a library or a source_set? https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g... https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.cc (right): https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:52: callback.Run(false); aside: this could be a cool signal in our page quality metric
Good to see this going in! https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.cc (right): https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:26: void BackgroundLoaderContents::CloseContents(content::WebContents* source) { Why have two methods that do the same thing? Are this and Cancel expected to diverge at some point? Are we just trying to better document the intent? Is this just implementing an existing interface and both methods are best handled by cancelling?
https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.cc (right): https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:36: return true; I wonder if it would be helpful to add a comment on what policy/intent this is targeting in these methods so maintainers have some better clue. Eg, maybe // Dialog prompts are not actionable in the background.
Updated per comments and added tests. There are no tests for Cancel and LoadURL, as they require a full WebContents to be created. If we only have mocks, then we end up testing implementation, which doesn't seem like something worthy of testing... https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... File components/offline_pages/content/background_loader/BUILD.gn (right): https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/BUILD.gn:9: static_library("background_loader") { On 2016/11/08 00:16:39, dewittj wrote: > do we want this to be a library or a source_set? > > https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g... I don't know, but the style guide seems to say that source sets are never wrong, so changed to that. Also, not sure why this is showing app_current_window_internal on the other side of diff :/ https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.cc (right): https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:23: url, content::Referrer(), ui::PAGE_TRANSITION_LINK, std::string()); On 2016/11/08 00:07:18, Dmitry Titov wrote: > Could you add comment on the choice of parameter values? > Also, the std::string() param - need to add comment on what it is. Done. https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:26: void BackgroundLoaderContents::CloseContents(content::WebContents* source) { On 2016/11/08 01:33:43, Pete Williamson wrote: > Why have two methods that do the same thing? Are this and Cancel expected to > diverge at some point? Are we just trying to better document the intent? Is > this just implementing an existing interface and both methods are best handled > by cancelling? Cancel() is our own - for when we want to fail early out of a request loop. CloseContents is inherited/overridden through the WebContentsDelegate, and actually shouldn't be called in vast majority of cases. I can see intent being something that differentiates the 2 - Cancel() is when we call fail out and CloseContents is when a different tab or other management closes this webcontents. I can see these diverging in the future - maybe when we are failing out, we want some sort of 'save progress' signal, or maybe we want to ignore CloseContents calls because any random tab shouldn't be able to just close us. https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:30: void BackgroundLoaderContents::Cancel() { On 2016/11/08 00:07:18, Dmitry Titov wrote: > The methods in cc file should be in the same order as they are in .h file. > (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md, > "Code Formatting") Done. https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:36: return true; On 2016/11/08 17:09:41, dougarnett wrote: > I wonder if it would be helpful to add a comment on what policy/intent this is > targeting in these methods so maintainers have some better clue. Eg, maybe > > // Dialog prompts are not actionable in the background. Done. https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:52: callback.Run(false); On 2016/11/08 00:07:18, Dmitry Titov wrote: > It'd be nice to look at callers of CanDownload and check if they can consume the > reentrant call to the callback. Often it is safer to PostTask with a callback > call to make sure it comes on a clean stack. Same for other callback.Run() calls > in this file. The WebContentsDelegate method comments say that the callback can be called/returned immediately https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:52: callback.Run(false); On 2016/11/08 00:16:39, dewittj wrote: > aside: this could be a cool signal in our page quality metric That's interesting. Perhaps not now, but we can add a signal to page quality once that is established.
https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... File components/offline_pages/content/background_loader/BUILD.gn (right): https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/BUILD.gn:9: static_library("background_loader") { On 2016/11/10 22:05:07, chili wrote: > On 2016/11/08 00:16:39, dewittj wrote: > > do we want this to be a library or a source_set? > > > > > https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g... > > I don't know, but the style guide seems to say that source sets are never wrong, > so changed to that. > > Also, not sure why this is showing app_current_window_internal on the other side > of diff :/ The app_current_... is due to Git similarity matching; apparently your file is pretty close to another already existing file. When you upload, you can say --similarity=99 (or something like that) to make it do more exact matching
lgtm https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.cc (right): https://codereview.chromium.org/2481443002/diff/20001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:36: return true; On 2016/11/10 22:05:08, chili wrote: > On 2016/11/08 17:09:41, dougarnett wrote: > > I wonder if it would be helpful to add a comment on what policy/intent this is > > targeting in these methods so maintainers have some better clue. Eg, maybe > > > > // Dialog prompts are not actionable in the background. > > Done. Thanks, I find those really helpful https://codereview.chromium.org/2481443002/diff/80001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.cc (right): https://codereview.chromium.org/2481443002/diff/80001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:40: // Do nothing. Other pages should not be able to close a background page. NOTREACHED() then?
lgtm
Thanks for all the comments! :) https://codereview.chromium.org/2481443002/diff/80001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.cc (right): https://codereview.chromium.org/2481443002/diff/80001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.cc:40: // Do nothing. Other pages should not be able to close a background page. On 2016/11/14 20:12:16, dougarnett wrote: > NOTREACHED() then? Done.
lgtm, thanks!
The CQ bit was checked by chili@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2016/11/15 02:45:03, Dmitry Titov wrote: > lgtm, thanks! LGTM!
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, dougarnett@chromium.org, dewittj@google.com, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2481443002/#ps120001 (title: "rebase-update, since there seem to be unrelated iOS issues before")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by chili@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...
chili@chromium.org changed reviewers: + blundell@chromium.org
+blundell for components/BUILD.gn owners
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
//components/BUILD.gn lgtm
The CQ bit was checked by chili@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 chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, dougarnett@chromium.org, dewittj@google.com, dimich@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2481443002/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Implementation (cc file) for background loader contents. BUG=659414 ========== to ========== Implementation (cc file) for background loader contents. BUG=659414 Committed: https://crrev.com/2e25132b6341e45f1704e9f1ea39d14c121273fe Cr-Commit-Position: refs/heads/master@{#433393} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2e25132b6341e45f1704e9f1ea39d14c121273fe Cr-Commit-Position: refs/heads/master@{#433393} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
