Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11273119:
Establish the content/components directory. (Closed)
|
Created:
8 years, 1 month ago by Jói Modified:
8 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, darin (slow to review), Ben Goodger (Google), android-webview-reviews_chromium.org, joth, browser-components-watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEstablish the content/components directory.
This is a home for components that have the Content Module as the
topmost layer they depend upon.
BUG=138280
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=165021
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address review comments. #Patch Set 3 : Address review comments. #Patch Set 4 : Address review comments. #
Messages
Total messages: 15 (0 generated)
Hi John, As discussed in email. Cait is getting close to ready to move the FaviconDownloadHelper out of chrome/, and the Android WebView guys want to move the couple of things that currently live under chrome/browser/component/, so it's time to establish this directory. The first component to move will establish the content/components/content_components.gyp file and set the pattern for .gypi files within components; it's hard to do that in a sensible way in this change, in a vacuum. Cheers, Jói
lgtm
lgtm
On 2012/10/30 17:27:56, Jói wrote: > Hi John, > > As discussed in email. Cait is getting close to ready to move the > FaviconDownloadHelper out of chrome/, and the Android WebView guys want to move > the couple of things that currently live under chrome/browser/component/, so > it's time to establish this directory. btw I'm wondering how you decide whether to move the favicon download code into a component vs making it part of content? i.e. an argument can be made that all embedders of content will want favicons, as they're part of the web platform, so it belongs in content. then an embedder of content has a delegate api to manage storage of the data? > > The first component to move will establish the > content/components/content_components.gyp file and set the pattern for .gypi > files within components; it's hard to do that in a sensible way in this change, > in a vacuum. > > Cheers, > Jói
On 2012/10/30 18:12:18, John Abd-El-Malek wrote: > On 2012/10/30 17:27:56, Jói wrote: > > Hi John, > > > > As discussed in email. Cait is getting close to ready to move the > > FaviconDownloadHelper out of chrome/, and the Android WebView guys want to > move > > the couple of things that currently live under chrome/browser/component/, so > > it's time to establish this directory. > > btw I'm wondering how you decide whether to move the favicon download code into > a component vs making it part of content? i.e. an argument can be made that all > embedders of content will want favicons, as they're part of the web platform, so > it belongs in content. then an embedder of content has a delegate api to manage > storage of the data? also, note I don't think this would change the work much. i.e. that code has to be extracted from chrome either way, which is the bulk of the work. it's more of a question about whether it should move into content\browser (and with a delegate in content\public\browser) or move into content\components\favicon in retrospect, for features like favicon when I was drawing the line between content/chrome, I should have been more granular and moved more of favicon code into content.
http://codereview.chromium.org/11273119/diff/1/content/components/README File content/components/README (right): http://codereview.chromium.org/11273119/diff/1/content/components/README#newc... content/components/README:20: content/components/foo/java - Android code One of these is not like the others. Why is java a peer to browser, common and renderer? It seems like java code could be browser-side only, renderer-side only, or used in both.
On 30 October 2012 18:18, <darin@chromium.org> wrote: > > http://codereview.chromium.**org/11273119/diff/1/content/** > components/README<http://codereview.chromium.org/11273119/diff/1/content/components/README> > File content/components/README (right): > > http://codereview.chromium.**org/11273119/diff/1/content/** > components/README#newcode20<http://codereview.chromium.org/11273119/diff/1/content/components/README#newcode20> > content/components/README:20: content/components/foo/java - Android code > One of these is not like the others. Why is java a peer to browser, > common and renderer? It seems like java code could be browser-side > only, renderer-side only, or used in both. > > Note that java already encodes the class's package name into the file path, and by convention we already put the browser vs renderer into the package name, so sub dividing the java trees any further doesn't actually help java code compartmentalization, but just deepens the already cumbersome paths and hurts compile speed e.g. https://src.chromium.org/viewvc/chrome/trunk/src/chrome/android/java/src/org/... see the chrome/browser on the end However, it does have 'android' in there, helping to cut the distinction of platform specific vs portable code. > http://codereview.chromium.**org/11273119/<http://codereview.chromium.org/112... > > -- > > >
On Tue, Oct 30, 2012 at 11:31 AM, Jonathan Dixon <joth@chromium.org> wrote: > > > > On 30 October 2012 18:18, <darin@chromium.org> wrote: > >> >> http://codereview.chromium.**org/11273119/diff/1/content/** >> components/README<http://codereview.chromium.org/11273119/diff/1/content/components/README> >> File content/components/README (right): >> >> http://codereview.chromium.**org/11273119/diff/1/content/** >> components/README#newcode20<http://codereview.chromium.org/11273119/diff/1/content/components/README#newcode20> >> content/components/README:20: content/components/foo/java - Android code >> One of these is not like the others. Why is java a peer to browser, >> common and renderer? It seems like java code could be browser-side >> only, renderer-side only, or used in both. >> >> > Note that java already encodes the class's package name into the file > path, and by convention we already put the browser vs renderer into the > package name, so sub dividing the java trees any further doesn't actually > help java code compartmentalization, but just deepens the already > cumbersome paths and hurts compile speed > > e.g. > https://src.chromium.org/viewvc/chrome/trunk/src/chrome/android/java/src/org/... > > see the chrome/browser on the end > > However, it does have 'android' in there, helping to cut the distinction > of platform specific vs portable code. > OK, then the README file should probably indicate the substructure of the java/ directory. > > > >> http://codereview.chromium.**org/11273119/<http://codereview.chromium.org/112... >> >> -- >> >> >> >
http://codereview.chromium.org/11273119/diff/1/content/components/README File content/components/README (right): http://codereview.chromium.org/11273119/diff/1/content/components/README#newc... content/components/README:24: be allowed to #include from content/public/browser. Please could you add a statement on what C++ namespace is to be used for components?
Since this is code under src/content/, shouldn't we just be using the content:: namespace? A potential negative of doing so is that it suggests that these components are part of content. I think that's OK since afterall they live under the src/content/ directory. I don't think a namespace like content_components:: adds much value. -Darin On Tue, Oct 30, 2012 at 11:48 AM, <jknotten@chromium.org> wrote: > > http://codereview.chromium.**org/11273119/diff/1/content/** > components/README<http://codereview.chromium.org/11273119/diff/1/content/components/README> > File content/components/README (right): > > http://codereview.chromium.**org/11273119/diff/1/content/** > components/README#newcode24<http://codereview.chromium.org/11273119/diff/1/content/components/README#newcode24> > content/components/README:24: be allowed to #include from > content/public/browser. > Please could you add a statement on what C++ namespace is to be used for > components? > > http://codereview.chromium.**org/11273119/<http://codereview.chromium.org/112... >
John: Thanks, perhaps the Favicon one should go into content rather than be a content component. I guess it depends whether you feel it is an optional component on top of content or should be part of content. Probably the latter? Darin: I will clean up the java/ thing in the comments. Agreed that I think we should probably just use content:: namespace. New patch a little later. Cheers, Jói On Tue, Oct 30, 2012 at 8:04 PM, Darin Fisher <darin@chromium.org> wrote: > Since this is code under src/content/, shouldn't we just be using the > content:: namespace? A potential negative of doing so is that it suggests > that these components are part of content. I think that's OK since afterall > they live under the src/content/ directory. I don't think a namespace like > content_components:: adds much value. > > -Darin > > > > On Tue, Oct 30, 2012 at 11:48 AM, <jknotten@chromium.org> wrote: >> >> >> http://codereview.chromium.org/11273119/diff/1/content/components/README >> File content/components/README (right): >> >> >> http://codereview.chromium.org/11273119/diff/1/content/components/README#newc... >> content/components/README:24: be allowed to #include from >> content/public/browser. >> Please could you add a statement on what C++ namespace is to be used for >> components? >> >> http://codereview.chromium.org/11273119/ > >
On Tue, Oct 30, 2012 at 1:12 PM, Jói Sigurðsson <joi@chromium.org> wrote: > John: Thanks, perhaps the Favicon one should go into content rather > than be a content component. I guess it depends whether you feel it > is an optional component on top of content or should be part of > content. Probably the latter? > I think given that content already knows about favicon (see http://code.google.com/p/chromium/source/search?q=file%3Acontent+favicon&orig...) and that it's something that every browser will need, it belongs in content. > Darin: I will clean up the java/ thing in the comments. Agreed that > I think we should probably just use content:: namespace. > btw there's precedent for this, i.e. content shell is in content\shell and is in the content namespace, but it's not part of the content library > New patch a little later. > > Cheers, > Jói > > > On Tue, Oct 30, 2012 at 8:04 PM, Darin Fisher <darin@chromium.org> wrote: > > Since this is code under src/content/, shouldn't we just be using the > > content:: namespace? A potential negative of doing so is that it > suggests > > that these components are part of content. I think that's OK since > afterall > > they live under the src/content/ directory. I don't think a namespace > like > > content_components:: adds much value. > > > > -Darin > > > > > > > > On Tue, Oct 30, 2012 at 11:48 AM, <jknotten@chromium.org> wrote: > >> > >> > >> > http://codereview.chromium.org/11273119/diff/1/content/components/README > >> File content/components/README (right): > >> > >> > >> > http://codereview.chromium.org/11273119/diff/1/content/components/README#newc... > >> content/components/README:24: be allowed to #include from > >> content/public/browser. > >> Please could you add a statement on what C++ namespace is to be used for > >> components? > >> > >> http://codereview.chromium.org/11273119/ > > > > >
Darin, PTAL - I fixed the README to add details on the namespace and explain the android/ (previously java/, my mistake) subdirectory if present. Cheers, Jói On Tue, Oct 30, 2012 at 8:23 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Tue, Oct 30, 2012 at 1:12 PM, Jói Sigurðsson <joi@chromium.org> wrote: >> >> John: Thanks, perhaps the Favicon one should go into content rather >> than be a content component. I guess it depends whether you feel it >> is an optional component on top of content or should be part of >> content. Probably the latter? > > > I think given that content already knows about favicon (see > http://code.google.com/p/chromium/source/search?q=file%3Acontent+favicon&orig...) > and that it's something that every browser will need, it belongs in content. > >> >> Darin: I will clean up the java/ thing in the comments. Agreed that >> I think we should probably just use content:: namespace. > > > btw there's precedent for this, i.e. content shell is in content\shell and > is in the content namespace, but it's not part of the content library > >> >> New patch a little later. >> >> Cheers, >> Jói >> >> >> On Tue, Oct 30, 2012 at 8:04 PM, Darin Fisher <darin@chromium.org> wrote: >> > Since this is code under src/content/, shouldn't we just be using the >> > content:: namespace? A potential negative of doing so is that it >> > suggests >> > that these components are part of content. I think that's OK since >> > afterall >> > they live under the src/content/ directory. I don't think a namespace >> > like >> > content_components:: adds much value. >> > >> > -Darin >> > >> > >> > >> > On Tue, Oct 30, 2012 at 11:48 AM, <jknotten@chromium.org> wrote: >> >> >> >> >> >> >> >> http://codereview.chromium.org/11273119/diff/1/content/components/README >> >> File content/components/README (right): >> >> >> >> >> >> >> >> http://codereview.chromium.org/11273119/diff/1/content/components/README#newc... >> >> content/components/README:24: be allowed to #include from >> >> content/public/browser. >> >> Please could you add a statement on what C++ namespace is to be used >> >> for >> >> components? >> >> >> >> http://codereview.chromium.org/11273119/ >> > >> > > >
LGTM
Thanks all, committed as r165021. Cheers, Jói On Tue, Oct 30, 2012 at 9:45 PM, <darin@chromium.org> wrote: > LGTM > > http://codereview.chromium.org/11273119/ > > -- > > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
