Move webpreferences.* from webkit/ to content/

Move webpreferences.* from webkit/ to content/

BUG=338338
TEST=None
R=jam@chromium.org
TBR=boliu@chromium.org # for android_webview
TBR=bauerb@chromium.org # for components/plugins
TBR=dmichael@chromium.org # for ppapi

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281978

[tfarina, 2014-06-28 19:59:20]

https://codereview.chromium.org/357203003/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/357203003/diff/1/content/content_common.gypi#...
content/content_common.gypi:128: 'public/common/webpreferences.cc',
had to keep it webpreferences.cc because there is already an
content/renderer/web_preferences.cc

[jam, 2014-06-30 04:36:59]

also do my comment from https://codereview.chromium.org/147373005/, ppapi can't
depend on content

https://codereview.chromium.org/357203003/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/357203003/diff/1/content/content_common.gypi#...
content/content_common.gypi:128: 'public/common/webpreferences.cc',
On 2014/06/28 19:59:20, tfarina wrote:
> had to keep it webpreferences.cc because there is already an
> content/renderer/web_preferences.cc

we shouldn't have webpreferences and web_preferences, that is just confusing.

keep the one in content\public\common to be web_preferences.* as before. remove
the header in content\public\renderer, and move that method to RenderView (i.e.
just ApplyWebPreferences(web_prefs)

[tfarina, 2014-07-01 02:35:36]

all done. ptal

[tfarina, 2014-07-02 18:10:19]

John, ping?

[jam, 2014-07-02 18:33:24]

lgtm, sorry I missed the earlier ping

https://codereview.chromium.org/357203003/diff/280001/content/public/renderer...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/357203003/diff/280001/content/public/renderer...
content/public/renderer/render_view.h:76: virtual void ApplyWebPreferences(const
WebPreferences& preferences) = 0;
nit: please add a comment here

actually I had forgotten about SetWebKitPreferences. I wonder if we really need
both of these methods? It would be great in a followup to try to hide Apply in
the impl only, and to have external callers call Set only.

[tfarina, 2014-07-02 23:57:36]

TBRing owners:

android_webview -> boliu
components/plugins -> bauerb
ppapi -> dmichael

Thanks,

https://codereview.chromium.org/357203003/diff/280001/content/public/renderer...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/357203003/diff/280001/content/public/renderer...
content/public/renderer/render_view.h:76: virtual void ApplyWebPreferences(const
WebPreferences& preferences) = 0;
On 2014/07/02 18:33:23, jam wrote:
> nit: please add a comment here
> 
> actually I had forgotten about SetWebKitPreferences. I wonder if we really
need
> both of these methods? It would be great in a followup to try to hide Apply in
> the impl only, and to have external callers call Set only.

Done.

[boliu, 2014-07-03 00:17:35]

https://codereview.chromium.org/357203003/diff/300001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/357203003/diff/300001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:40: using
content::WebPreferences;
This is only used at once place in this file. Prefer simply using the full name
that that one place.

https://codereview.chromium.org/357203003/diff/300001/android_webview/rendere...
File android_webview/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/357203003/diff/300001/android_webview/rendere...
android_webview/renderer/print_web_view_helper.cc:673:
render_view_->ApplyWebPreferences(prefs);
Does this work?

frame_->GetRenderView()->ApplyWebPreferences(prefs)

Also note that this file should be kept in sync with the one under chrome

https://codereview.chromium.org/357203003/diff/300001/chrome/browser/android/...
File chrome/browser/android/voice_search_tab_helper.cc (right):

https://codereview.chromium.org/357203003/diff/300001/chrome/browser/android/...
chrome/browser/android/voice_search_tab_helper.cc:14: using
content::WebPreferences;
ditto about using it only once

[tfarina, 2014-07-03 00:50:14]

https://codereview.chromium.org/357203003/diff/300001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/357203003/diff/300001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:40: using
content::WebPreferences;
On 2014/07/03 00:17:35, boliu wrote:
> This is only used at once place in this file. Prefer simply using the full
name
> that that one place.

Done.

https://codereview.chromium.org/357203003/diff/300001/android_webview/rendere...
File android_webview/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/357203003/diff/300001/android_webview/rendere...
android_webview/renderer/print_web_view_helper.cc:673:
render_view_->ApplyWebPreferences(prefs);
On 2014/07/03 00:17:35, boliu wrote:
> Does this work?
> 
I don't think this would work. As far as I can see |frame_| is of type
blink::WebLocalFrame.

https://codereview.chromium.org/357203003/diff/300001/chrome/browser/android/...
File chrome/browser/android/voice_search_tab_helper.cc (right):

https://codereview.chromium.org/357203003/diff/300001/chrome/browser/android/...
chrome/browser/android/voice_search_tab_helper.cc:14: using
content::WebPreferences;
On 2014/07/03 00:17:35, boliu wrote:
> ditto about using it only once

Done.

[boliu, 2014-07-03 00:59:14]

https://codereview.chromium.org/357203003/diff/300001/android_webview/rendere...
File android_webview/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/357203003/diff/300001/android_webview/rendere...
android_webview/renderer/print_web_view_helper.cc:673:
render_view_->ApplyWebPreferences(prefs);
On 2014/07/03 00:50:14, tfarina wrote:
> On 2014/07/03 00:17:35, boliu wrote:
> > Does this work?
> > 
> I don't think this would work. As far as I can see |frame_| is of type
> blink::WebLocalFrame.

Oops, my bad. How about...

content::RenderView::FromWebView(frame_->view()) ?

[Bernhard Bauer, 2014-07-03 09:56:59]

components/plugins/ LGTM

[tfarina, 2014-07-03 13:19:36]

boliu, ptal.

https://codereview.chromium.org/357203003/diff/300001/android_webview/rendere...
File android_webview/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/357203003/diff/300001/android_webview/rendere...
android_webview/renderer/print_web_view_helper.cc:673:
render_view_->ApplyWebPreferences(prefs);
On 2014/07/03 00:59:13, boliu wrote:
> On 2014/07/03 00:50:14, tfarina wrote:
> > On 2014/07/03 00:17:35, boliu wrote:
> > > Does this work?
> > > 
> > I don't think this would work. As far as I can see |frame_| is of type
> > blink::WebLocalFrame.
> 
> Oops, my bad. How about...
> 
> content::RenderView::FromWebView(frame_->view()) ?

Done.

[boliu, 2014-07-03 14:13:33]

aw lgtm

[tfarina, 2014-07-03 16:06:49]

On 2014/07/03 14:13:33, boliu wrote:
> aw lgtm
browser_tests didn't like this change, see below (and red bots):

PrintWebViewHelperPreviewTest.OnPrintPreviewForSelectedText (run #1):
[ RUN      ] PrintWebViewHelperPreviewTest.OnPrintPreviewForSelectedText
Received signal 11 SEGV_MAPERR 000000000000
#0 0x0000017a69fe base::debug::StackTrace::StackTrace()
#1 0x0000017a6c18 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fc5f2200cb0 \u003Cunknown>
#3 0x00000170c6b1 printing::PrepareFrameAndViewForPrint::CopySelection()
#4 0x00000170ea18 printing::PrintWebViewHelper::PrepareFrameForPreviewDocument()
#5 0x00000170ec83 printing::PrintWebViewHelper::OnPrintPreview()
#6 0x000000ef607a
printing::PrintWebViewHelperPreviewTest_OnPrintPreviewForSelectedText_Test::TestBody()
#7 0x000001bfd4ca testing::Test::Run()
#8 0x000001bfd58a testing::TestInfo::Run()
#9 0x000001bfd6cf testing::TestCase::Run()
#10 0x000001bfd95f testing::internal::UnitTestImpl::RunAllTests()
#11 0x000001bf7903 testing::UnitTest::Run()
#12 0x00000182c03e base::TestSuite::Run()
#13 0x000000f35745 ChromeBrowserTestSuiteRunner::RunTestSuite()
#14 0x000004e8f63c content::LaunchTests()
#15 0x00000177c665 LaunchChromeTests()
#16 0x00000048870b main

Are you OK with me reverting to what I had previously?

[boliu, 2014-07-03 16:49:32]

https://codereview.chromium.org/357203003/diff/330001/android_webview/rendere...
File android_webview/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/357203003/diff/330001/android_webview/rendere...
android_webview/renderer/print_web_view_helper.cc:674:
->ApplyWebPreferences(prefs);
Ohh, now I understand. Printing is creating its own blink::WebView with this
PrepareFrameAndViewForPrint as the WebViewClient. (For normal pages,
WebViewClient is RenderViewImpl) That's why RenderView::FromWebView is returning
NULL here causing the crash. Good thing we have tests :)

This also means your original change is not correct either, because it was
applying the preferences on the *wrong* blink::WebView.

The only way I see to avoid code duplication is keep the original
content::ApplyWebPreferences as a standalone function that printing can call
here.

Sorry for the back and forth...

[tfarina, 2014-07-03 17:25:34]

https://codereview.chromium.org/357203003/diff/330001/android_webview/rendere...
File android_webview/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/357203003/diff/330001/android_webview/rendere...
android_webview/renderer/print_web_view_helper.cc:674:
->ApplyWebPreferences(prefs);
On 2014/07/03 16:49:32, boliu wrote:
> Ohh, now I understand. Printing is creating its own blink::WebView with this
> PrepareFrameAndViewForPrint as the WebViewClient. (For normal pages,
> WebViewClient is RenderViewImpl) That's why RenderView::FromWebView is
returning
> NULL here causing the crash. Good thing we have tests :)
> 
> This also means your original change is not correct either, because it was
> applying the preferences on the *wrong* blink::WebView.
> 
> The only way I see to avoid code duplication is keep the original
> content::ApplyWebPreferences as a standalone function that printing can call
> here.
>
OK.

That is a design decision.

John, what is your input here?
 
I can't have ApplyWebPreferences in content/public/common because it uses
content/renderer/net_info_helper.h. And I think I can't move WebPreferences
struct into content/public/renderer because it is used in common, browser...

[dmichael (off chromium), 2014-07-07 17:03:35]

pepper lgtm

https://codereview.chromium.org/357203003/diff/330001/content/renderer/pepper...
File content/renderer/pepper/ppapi_preferences_builder.h (right):

https://codereview.chromium.org/357203003/diff/330001/content/renderer/pepper...
content/renderer/pepper/ppapi_preferences_builder.h:19: };
nit: It would be good to add
 private:
  DISALLOW_COPY_AND_ASSIGN(PpapiPreferencesBuilder());

...to prevent misuse, since it appears there's no reason to construct one. That
not only takes care of copy and assign, but also implicitly ensures the compiler
won't give us a default constructor.

[tfarina, 2014-07-09 01:29:49]

John suggested making it a static method on RenderView. Which I did in patch set
19. Pushing to CQ.

[tfarina, 2014-07-09 01:29:52]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-07-09 01:31:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tfarina@chromium.org/357203003/350001

[tfarina, 2014-07-09 01:48:30]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-07-09 01:49:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tfarina@chromium.org/357203003/370001

[commit-bot: I haz the power, 2014-07-09 06:25:19]

Change committed as 281978

[tfarina, 2014-07-15 00:47:32]

https://codereview.chromium.org/357203003/diff/330001/content/renderer/pepper...
File content/renderer/pepper/ppapi_preferences_builder.h (right):

https://codereview.chromium.org/357203003/diff/330001/content/renderer/pepper...
content/renderer/pepper/ppapi_preferences_builder.h:19: };
On 2014/07/07 17:03:34, dmichael wrote:
> nit: It would be good to add
>  private:
>   DISALLOW_COPY_AND_ASSIGN(PpapiPreferencesBuilder());
> 
> ...to prevent misuse, since it appears there's no reason to construct one.
That
> not only takes care of copy and assign, but also implicitly ensures the
compiler
> won't give us a default constructor.

I will address this separately. None of the builder.h files in content/ were
using DISALLOW_IMPLICIT_CONSTRUCTORS.

So I will make a patch to make all of them use DISALLOW_IMPLICIT_CONSTRUCTORS.