Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10908099:
Copy renderer preferences from embedder to guest. (Closed)
|
Created:
8 years, 3 months ago by lazyboy Modified:
8 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
http://git.chromium.org/chromium/src.git@tmp Visibility:
Public. |
Description[gtk/aura]: This fixes selection color, caret blinking for guests.
BUG=146582
Tested=GTK+Aura; Sanity check on OSX (no change).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159182
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add focus ring color prefs too. #
Total comments: 1
Patch Set 3 : Copy *all* the preferences. #Patch Set 4 : Add comments so it's obvious why we need to copy renderer prefs (gtk+aura). #
Total comments: 7
Patch Set 5 : Address comments, sync@LKGR #Patch Set 6 : Sync to HEAD, diff conflict otherwise. #Messages
Total messages: 21 (0 generated)
http://codereview.chromium.org/10908099/diff/1/content/browser/browser_plugin... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): http://codereview.chromium.org/10908099/diff/1/content/browser/browser_plugin... content/browser/browser_plugin/browser_plugin_embedder.cc:117: #if defined(TOOLKIT_GTK) Why should this be GTK only? Why not copy all the prefs on all platforms? There might be a good reason not to do this although I can't think of it off the top of my head.
http://codereview.chromium.org/10908099/diff/1/content/browser/browser_plugin... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): http://codereview.chromium.org/10908099/diff/1/content/browser/browser_plugin... content/browser/browser_plugin/browser_plugin_embedder.cc:117: #if defined(TOOLKIT_GTK) On 2012/09/06 21:21:07, Fady Samuel wrote: > Why should this be GTK only? Why not copy all the prefs on all platforms? There > might be a good reason not to do this although I can't think of it off the top > of my head. It's only GTK that uses the renderer_prefs->color values from what I read the code: http://108.166.104.54/source/xref/chromium/src/content/public/common/renderer..., Also selection color works fine without the changes here on win and mac.
On 2012/09/08 02:38:55, lazyboy wrote: > http://codereview.chromium.org/10908099/diff/1/content/browser/browser_plugin... > File content/browser/browser_plugin/browser_plugin_embedder.cc (right): > > http://codereview.chromium.org/10908099/diff/1/content/browser/browser_plugin... > content/browser/browser_plugin/browser_plugin_embedder.cc:117: #if > defined(TOOLKIT_GTK) > On 2012/09/06 21:21:07, Fady Samuel wrote: > > Why should this be GTK only? Why not copy all the prefs on all platforms? > There > > might be a good reason not to do this although I can't think of it off the top > > of my head. > > It's only GTK that uses the renderer_prefs->color values from what I read the > code: > http://108.166.104.54/source/xref/chromium/src/content/public/common/renderer..., > Also selection color works fine without the changes here on win and mac. Can you take a look? Ty.
https://chromiumcodereview.appspot.com/10908099/diff/4001/content/browser/bro... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10908099/diff/4001/content/browser/bro... content/browser/browser_plugin/browser_plugin_embedder.cc:102: #if defined(TOOLKIT_GTK) Is it worthwhile to just copy the struct on all platforms? Does this break anything?
Ok, I'm copying all the renderer preferences now instead of selectively copying which were broken. Checked on GTK and Aura, seems to work. Sanity check on OSX, things still work.
On 2012/09/24 19:14:41, lazyboy wrote: > Ok, I'm copying all the renderer preferences now instead of selectively copying > which were broken. > > Checked on GTK and Aura, seems to work. > Sanity check on OSX, things still work. LGTM. Please get Charlie to review this as OWNER.
Added comments as Fady suggested. Adding Charlie for owners. Thanks.
Seems reasonable, but one question. https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... content/browser/browser_plugin/browser_plugin_embedder.cc:105: *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs(); Just curious why this is necessary. Where does the embedder's web_contents() have its prefs assigned? I'm not sure why the guest's WebContents isn't using the same approach.
https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... content/browser/browser_plugin/browser_plugin_embedder.cc:105: *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs(); I believe RenderViewHostImpl.CreateRenderView() gets the preferences: delegate_->GetRendererPrefs(GetProcess()->GetBrowserContext()) For guests, I don't know why they don't default to the same values as the embedder. (Win and OSX doesn't seem to care about these values). On 2012/09/25 19:15:25, creis wrote: > Just curious why this is necessary. Where does the embedder's web_contents() > have its prefs assigned? I'm not sure why the guest's WebContents isn't using > the same approach.
https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... content/browser/browser_plugin/browser_plugin_embedder.cc:105: *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs(); On 2012/09/25 19:26:20, lazyboy wrote: > I believe RenderViewHostImpl.CreateRenderView() gets the preferences: > > delegate_->GetRendererPrefs(GetProcess()->GetBrowserContext()) > > For guests, I don't know why they don't default to the same values as the > embedder. > > (Win and OSX doesn't seem to care about these values). > > > On 2012/09/25 19:15:25, creis wrote: > > Just curious why this is necessary. Where does the embedder's web_contents() > > have its prefs assigned? I'm not sure why the guest's WebContents isn't using > > the same approach. > That's probably where the prefs are passed to the new renderer, but it doesn't explain why the WebContents (i.e., delegate_) has the right settings in the embedder but not in the guest. Sorry to push on this, but I'm worried that we might be missing some other initialization of guest WebContentses if we just paper over this. Can you find where the embedder WebContents is getting the correct values from?
https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... content/browser/browser_plugin/browser_plugin_embedder.cc:105: *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs(); For the embedder, the TabContents's PrefsTabHelper is responsible for retrieving these settings from system. (that's in chrome/ui). The guest's WC doesn't have a TabContents. From Fady: Because BrowserPlugin was designed to operate entirely in the content layer (as per jam@'s suggestions). On 2012/09/25 21:04:36, creis wrote: > On 2012/09/25 19:26:20, lazyboy wrote: > > I believe RenderViewHostImpl.CreateRenderView() gets the preferences: > > > > delegate_->GetRendererPrefs(GetProcess()->GetBrowserContext()) > > > > For guests, I don't know why they don't default to the same values as the > > embedder. > > > > (Win and OSX doesn't seem to care about these values). > > > > > > On 2012/09/25 19:15:25, creis wrote: > > > Just curious why this is necessary. Where does the embedder's > web_contents() > > > have its prefs assigned? I'm not sure why the guest's WebContents isn't > using > > > the same approach. > > > > That's probably where the prefs are passed to the new renderer, but it doesn't > explain why the WebContents (i.e., delegate_) has the right settings in the > embedder but not in the guest. > > Sorry to push on this, but I'm worried that we might be missing some other > initialization of guest WebContentses if we just paper over this. Can you find > where the embedder WebContents is getting the correct values from?
https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... content/browser/browser_plugin/browser_plugin_embedder.cc:105: *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs(); On 2012/09/25 21:21:38, lazyboy wrote: > For the embedder, the TabContents's PrefsTabHelper is responsible for retrieving > these settings from system. (that's in chrome/ui). > The guest's WC doesn't have a TabContents. From Fady: Because BrowserPlugin was > designed to operate entirely in the content layer (as per jam@'s suggestions). Ah, now we're getting somewhere. It's interesting that the guest WebContents has no TabContents. On the one hand, that's the right thing to do, since there's no visible, actual tab for it. On the other hand, there's tons of things in there that the guest will presumably be missing: ZoomController, FindTabHelper, etc. Are we ok with just doing the prefs piecemeal like this and missing everything else in the guest? Or are we going to gradually have to add things here as we realize more is broken? (Maybe some/most of the stuff in TabContents won't make sense in a guest, and maybe it won't be a problem as TabContents goes away in http://crbug.com/107201.)
On 2012/09/25 23:19:25, creis wrote: > https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... > File content/browser/browser_plugin/browser_plugin_embedder.cc (right): > > https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... > content/browser/browser_plugin/browser_plugin_embedder.cc:105: > *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs(); > On 2012/09/25 21:21:38, lazyboy wrote: > > For the embedder, the TabContents's PrefsTabHelper is responsible for > retrieving > > these settings from system. (that's in chrome/ui). > > The guest's WC doesn't have a TabContents. From Fady: Because BrowserPlugin > was > > designed to operate entirely in the content layer (as per jam@'s suggestions). > > Ah, now we're getting somewhere. It's interesting that the guest WebContents > has no TabContents. On the one hand, that's the right thing to do, since > there's no visible, actual tab for it. On the other hand, there's tons of > things in there that the guest will presumably be missing: ZoomController, > FindTabHelper, etc. > > Are we ok with just doing the prefs piecemeal like this and missing everything > else in the guest? Or are we going to gradually have to add things here as we > realize more is broken? (Maybe some/most of the stuff in TabContents won't make > sense in a guest, and maybe it won't be a problem as TabContents goes away in > http://crbug.com/107201.) Do these things even make sense in the context of Chrome Apps? I feel like even if we want to implement some kind of find feature for guests, it would need to be managed by the embedder and there wouldn't be too much code reuse with existing find mechanisms?
On 2012/09/26 00:17:19, Fady Samuel wrote: > On 2012/09/25 23:19:25, creis wrote: > > > https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... > > File content/browser/browser_plugin/browser_plugin_embedder.cc (right): > > > > > https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... > > content/browser/browser_plugin/browser_plugin_embedder.cc:105: > > *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs(); > > On 2012/09/25 21:21:38, lazyboy wrote: > > > For the embedder, the TabContents's PrefsTabHelper is responsible for > > retrieving > > > these settings from system. (that's in chrome/ui). > > > The guest's WC doesn't have a TabContents. From Fady: Because BrowserPlugin > > was > > > designed to operate entirely in the content layer (as per jam@'s > suggestions). > > > > Ah, now we're getting somewhere. It's interesting that the guest WebContents > > has no TabContents. On the one hand, that's the right thing to do, since > > there's no visible, actual tab for it. On the other hand, there's tons of > > things in there that the guest will presumably be missing: ZoomController, > > FindTabHelper, etc. > > > > Are we ok with just doing the prefs piecemeal like this and missing everything > > else in the guest? Or are we going to gradually have to add things here as we > > realize more is broken? (Maybe some/most of the stuff in TabContents won't > make > > sense in a guest, and maybe it won't be a problem as TabContents goes away in > > http://crbug.com/107201.) > > Do these things even make sense in the context of Chrome Apps? I feel like even > if we want to implement some kind of find feature for guests, it would need to > be managed by the embedder and there wouldn't be too much code reuse with > existing find mechanisms? Maybe Find doesn't, but what about Zoom? Or if none of these things should be applied to guests, then why do we care about copying over the prefs? (For comparison, we're not applying content_settings, which is arguably similar to prefs.) I think I'm ok with this change, but I don't yet understand why prefs is a special case and nothing else in tab_contents.h needs to be applied. At the least, we should add a comment saying that the prefs are inherited from the embedder, but nothing else is. (I would love for that comment to say why, if possible.)
On 2012/09/26 00:27:57, creis wrote: > On 2012/09/26 00:17:19, Fady Samuel wrote: > > On 2012/09/25 23:19:25, creis wrote: > > > > > > https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... > > > File content/browser/browser_plugin/browser_plugin_embedder.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... > > > content/browser/browser_plugin/browser_plugin_embedder.cc:105: > > > *guest_renderer_prefs = *web_contents()->GetMutableRendererPrefs(); > > > On 2012/09/25 21:21:38, lazyboy wrote: > > > > For the embedder, the TabContents's PrefsTabHelper is responsible for > > > retrieving > > > > these settings from system. (that's in chrome/ui). > > > > The guest's WC doesn't have a TabContents. From Fady: Because > BrowserPlugin > > > was > > > > designed to operate entirely in the content layer (as per jam@'s > > suggestions). > > > > > > Ah, now we're getting somewhere. It's interesting that the guest > WebContents > > > has no TabContents. On the one hand, that's the right thing to do, since > > > there's no visible, actual tab for it. On the other hand, there's tons of > > > things in there that the guest will presumably be missing: ZoomController, > > > FindTabHelper, etc. > > > > > > Are we ok with just doing the prefs piecemeal like this and missing > everything > > > else in the guest? Or are we going to gradually have to add things here as > we > > > realize more is broken? (Maybe some/most of the stuff in TabContents won't > > make > > > sense in a guest, and maybe it won't be a problem as TabContents goes away > in > > > http://crbug.com/107201.) > > > > Do these things even make sense in the context of Chrome Apps? I feel like > even > > if we want to implement some kind of find feature for guests, it would need to > > be managed by the embedder and there wouldn't be too much code reuse with > > existing find mechanisms? > > Maybe Find doesn't, but what about Zoom? Or if none of these things should be > applied to guests, then why do we care about copying over the prefs? (For > comparison, we're not applying content_settings, which is arguably similar to > prefs.) > > I think I'm ok with this change, but I don't yet understand why prefs is a > special case and nothing else in tab_contents.h needs to be applied. At the > least, we should add a comment saying that the prefs are inherited from the > embedder, but nothing else is. (I would love for that comment to say why, if > possible.) For platform specific colors such as selection highlight color.
LGTM with one request. https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... content/browser/browser_plugin/browser_plugin_embedder.cc:102: // Copy renderer preferences to guest. For GTK and Aura this is necessary to I'd like to make a note here that this is intentionally the sole thing we're copying from the embedder's TabContents, so that we remember where it came from (in case we decide we're missing something later). Something as simple as changing this to: Copy renderer preferences (and nothing else) from the embedder's TabContents to the guest.
Thanks for the review. https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://chromiumcodereview.appspot.com/10908099/diff/13001/content/browser/br... content/browser/browser_plugin/browser_plugin_embedder.cc:102: // Copy renderer preferences to guest. For GTK and Aura this is necessary to On 2012/09/27 19:01:32, creis wrote: > I'd like to make a note here that this is intentionally the sole thing we're > copying from the embedder's TabContents, so that we remember where it came from > (in case we decide we're missing something later). Something as simple as > changing this to: > > Copy renderer preferences (and nothing else) from the embedder's TabContents to > the guest. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10908099/13002
Failed to apply patch for content/browser/browser_plugin/browser_plugin_embedder.cc: While running patch -p1 --forward --force; patching file content/browser/browser_plugin/browser_plugin_embedder.cc Hunk #1 FAILED at 97. 1 out of 1 hunk FAILED -- saving rejects to file content/browser/browser_plugin/browser_plugin_embedder.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lazyboy@chromium.org/10908099/22001
Change committed as 159182 |
