Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466273007:
Remove list of observers from background loader and associated methods. Also small compile-friendl… (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. |
DescriptionRemove list of observers from background loader and associated methods. Also small compile-friendly updates
BUG=659414
Committed: https://crrev.com/389d0808461f9c19a3a664dbf8a1edfa9672a164
Cr-Commit-Position: refs/heads/master@{#429700}
Patch Set 1 : New background loader contents, updated to match latest changes in design #
Total comments: 4
Patch Set 2 : fully compilable h file #Messages
Total messages: 16 (6 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
chili@chromium.org changed reviewers: + dewittj@chromium.org, dimich@chromium.org, fgorski@chromium.org, petewil@chromium.org
New (and improved) .h file for background loader :)
lgtm https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:35: WebContents* getWebContents(); Just curious, how come we have content::WebContents in some places, and just WebContents in other places?
Cathy I recommend you try to build it, at least locally, to catch some quirks. https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:35: WebContents* getWebContents(); On 2016/11/02 20:46:43, Pete Williamson wrote: > Just curious, how come we have content::WebContents in some places, and just > WebContents in other places? Likely, it is not compiled yet. GetWebContents() or web_contents() is a proper casing here.
lgtm
lgtm with a nit: https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:35: WebContents* getWebContents(); This looks like a trivial accessor to the member variable. I'd call it web_contents(). it can even be inlined. Should return content::WebContents*
https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... File components/offline_pages/content/background_loader/background_loader_contents.h (right): https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... components/offline_pages/content/background_loader/background_loader_contents.h:35: WebContents* getWebContents(); On 2016/11/02 22:07:39, fgorski wrote: > On 2016/11/02 20:46:43, Pete Williamson wrote: > > Just curious, how come we have content::WebContents in some places, and just > > WebContents in other places? > > Likely, it is not compiled yet. > > GetWebContents() or web_contents() is a proper casing here. It was compiled right before I added this -.- Of course the only new line I added wasn't compile-friendly.... I am able to compile with this h file now. PTAL
On 2016/11/03 18:57:20, chili wrote: > https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... > File > components/offline_pages/content/background_loader/background_loader_contents.h > (right): > > https://codereview.chromium.org/2466273007/diff/40001/components/offline_page... > components/offline_pages/content/background_loader/background_loader_contents.h:35: > WebContents* getWebContents(); > On 2016/11/02 22:07:39, fgorski wrote: > > On 2016/11/02 20:46:43, Pete Williamson wrote: > > > Just curious, how come we have content::WebContents in some places, and just > > > WebContents in other places? > > > > Likely, it is not compiled yet. > > > > GetWebContents() or web_contents() is a proper casing here. > > It was compiled right before I added this -.- Of course the only new line I > added wasn't compile-friendly.... > > I am able to compile with this h file now. PTAL 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, dimich@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2466273007/#ps60001 (title: "fully compilable h file")
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 #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Remove list of observers from background loader and associated methods. Also small compile-friendly updates BUG=659414 ========== to ========== Remove list of observers from background loader and associated methods. Also small compile-friendly updates BUG=659414 Committed: https://crrev.com/389d0808461f9c19a3a664dbf8a1edfa9672a164 Cr-Commit-Position: refs/heads/master@{#429700} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/389d0808461f9c19a3a664dbf8a1edfa9672a164 Cr-Commit-Position: refs/heads/master@{#429700} |
