Remove impl-side-painting from about:flags

Remove impl-side-painting from about:flags

This has been on everywhere for m39 and m40. People should not be
turning it off by this point, it will start to break soon.

BUG=169282
Committed: https://crrev.com/27f39f26c1636f9f09c79ab44f86e91b114b9a61
Cr-Commit-Position: refs/heads/master@{#312877}

[danakj, 2015-01-09 20:00:32]

danakj@chromium.org changed reviewers:
+ cjhopman@chromium.org

[danakj, 2015-01-09 20:00:32]

cjhopman@chromium.org: Please review changes in build/ios/

[danakj, 2015-01-09 20:08:11]

Updated the CL to remove the --enable-impl-side-painting flag. Just the disable
flag is left.

[danakj, 2015-01-09 20:10:25]

danakj@chromium.org changed reviewers:
+ mnaganov@chromium.org, sievers@chromium.org, sky@chromium.org

[danakj, 2015-01-09 20:10:25]

+sky for chrome/
+sievers for content/
+mnaganov for android_webview/

[no sievers, 2015-01-09 20:22:33]

lgtm

https://codereview.chromium.org/830273003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/830273003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1078:
command_line->AppendSwitch(switches::kDisableImplSidePainting);
So this was broken before in that it didn't propagate the cmdline to the
renderer?

[danakj, 2015-01-09 20:23:14]

https://codereview.chromium.org/830273003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/830273003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1078:
command_line->AppendSwitch(switches::kDisableImplSidePainting);
On 2015/01/09 20:22:32, sievers wrote:
> So this was broken before in that it didn't propagate the cmdline to the
> renderer?

No the renderer was looking for enable before. Now it'll look for disable
instead. See the RenderThreadImpl change.

[vmpstr, 2015-01-09 20:24:49]

lgtm

[sky, 2015-01-09 20:52:41]

https://codereview.chromium.org/830273003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/830273003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kDisableImplSidePainting,
Should this be removed too?

[mnaganov (inactive), 2015-01-09 21:21:20]

android_webview/ LGTM

I'm concerned about Android dbg test crashes. They all terminate due to this
CHECK:

[FATAL:render_thread_impl.cc(627)] Check failed: parsed_num_raster_threads.

Can you please take a look?

[danakj, 2015-01-09 21:32:52]

https://codereview.chromium.org/830273003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/830273003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1078:
command_line->AppendSwitch(switches::kDisableImplSidePainting);
On 2015/01/09 20:23:14, danakj wrote:
> On 2015/01/09 20:22:32, sievers wrote:
> > So this was broken before in that it didn't propagate the cmdline to the
> > renderer?
> 
> No the renderer was looking for enable before. Now it'll look for disable
> instead. See the RenderThreadImpl change.

Tho it appears that at least in the RenderThreadImplBrowserTest (which are not
actually browser tests...), implside painting was not previously enabled. Will
fix.

[danakj, 2015-01-09 21:40:26]

On Fri, Jan 9, 2015 at 1:21 PM, <mnaganov@chromium.org> wrote:

> android_webview/ LGTM
>
> I'm concerned about Android dbg test crashes. They all terminate due to
> this
> CHECK:
>
> [FATAL:render_thread_impl.cc(627)] Check failed:
> parsed_num_raster_threads.
>
> Can you please take a look?
>

Will do. That seems to imply implside painting was off in the renderer
though before..

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-01-09 21:40:38]

https://codereview.chromium.org/830273003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/830273003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/chrome_restart_request.cc:86:
::switches::kDisableImplSidePainting,
On 2015/01/09 20:52:41, sky wrote:
> Should this be removed too?

We're leaving this around for now.. for developers. But will come back and
remove soon, probably after 41 branch.

[danakj, 2015-01-09 22:06:38]

Fixed content_browsertests in the latest patch set by adding kNumRasterThreads
to the command line when it creates a RenderThreadImpl - we expect the browser
to always be passing this and it caused a DCHECK to just make a RenderThreadImpl
in the test without it.

[danakj, 2015-01-09 22:07:13]

On 2015/01/09 21:21:20, mnaganov (cr) wrote:
> android_webview/ LGTM
> 
> I'm concerned about Android dbg test crashes. They all terminate due to this
> CHECK:
> 
> [FATAL:render_thread_impl.cc(627)] Check failed: parsed_num_raster_threads.
> 
> Can you please take a look?

Looks like android tests were the same as on other platforms, should be green
now.

[cjhopman, 2015-01-09 22:26:31]

lgtm

[sky, 2015-01-09 23:05:01]

LGTM

[danakj, 2015-01-09 23:08:30]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-01-09 23:10:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/830273003/40001

[commit-bot: I haz the power, 2015-01-09 23:52:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-09 23:53:30]

Patchset 3 (id:??) landed as
https://crrev.com/c138f5dd7e317f141edf0e2f87a74c487174c172
Cr-Commit-Position: refs/heads/master@{#310895}

[f(malita), 2015-01-10 18:41:06]

On 2015/01/09 23:53:30, I haz the power (commit-bot) wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/c138f5dd7e317f141edf0e2f87a74c487174c172
> Cr-Commit-Position: refs/heads/master@{#310895}

Did this flip ISP on for drt/all layout tests? I'm seeing gobs of minor failures
pointing to this, for example

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=opa...
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg...

[f(malita), 2015-01-10 19:19:00]

On 2015/01/10 18:41:06, f(malita) wrote:
> On 2015/01/09 23:53:30, I haz the power (commit-bot) wrote:
> > Patchset 3 (id:??) landed as
> > https://crrev.com/c138f5dd7e317f141edf0e2f87a74c487174c172
> > Cr-Commit-Position: refs/heads/master@{#310895}
> 
> Did this flip ISP on for drt/all layout tests? I'm seeing gobs of minor
failures

Yup, I verified that without this CL the failures go away. Reverting, since the
Blink tree is in deep doo-doo (2000+ failures).

[f(malita), 2015-01-10 19:34:49]

Revert landed: https://codereview.chromium.org/846813002/

(the revert button was throwing a transient error).

[danakj, 2015-01-12 23:41:14]

This broke layout tests because they use --enable-impl-side-painting to override
--disable-impl-side-painting which is always passed for layout tests currently.

Changed this by moving the --enable flag over to the content shell switches
(instead of global content switches) and have content shell only pass --disable
if --enable is not present. This way only content shell needs to know about
this.

[piman, 2015-01-13 00:04:15]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2015-01-13 00:04:16]

lgtm

[danakj, 2015-01-13 00:47:01]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-01-13 00:47:16]

Green blink bot :D

[commit-bot: I haz the power, 2015-01-13 00:48:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/830273003/60001

[commit-bot: I haz the power, 2015-01-13 01:32:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-13 01:33:30]

Patchset 4 (id:??) landed as
https://crrev.com/a0b3c5b4bd504b6c699984149bd73ecf1fdc4e44
Cr-Commit-Position: refs/heads/master@{#311170}

[f(malita), 2015-01-22 19:09:19]

On 2015/01/13 01:33:30, I haz the power (commit-bot) wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/a0b3c5b4bd504b6c699984149bd73ecf1fdc4e44
> Cr-Commit-Position: refs/heads/master@{#311170}

Hmm, looks like this was reverted again:
https://codereview.chromium.org/846213003

:(

[danakj, 2015-01-22 19:21:51]

Err I did not see that. Well I will just wait for enne@'s CL then.

On Thu, Jan 22, 2015 at 11:09 AM, <fmalita@chromium.org> wrote:

> On 2015/01/13 01:33:30, I haz the power (commit-bot) wrote:
>
>> Patchset 4 (id:??) landed as
>> https://crrev.com/a0b3c5b4bd504b6c699984149bd73ecf1fdc4e44
>> Cr-Commit-Position: refs/heads/master@{#311170}
>>
>
> Hmm, looks like this was reverted again:
> https://codereview.chromium.org/846213003
>
> :(
>
> https://codereview.chromium.org/830273003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-01-22 19:26:17]

Looks like https://codereview.chromium.org/774783003/ landed so now layout tests
should behave with the same defaults as everything else, so I'll just try this
agian without the content shell changes.

[danakj, 2015-01-23 17:13:23]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-01-23 17:13:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/830273003/80001

[commit-bot: I haz the power, 2015-01-23 18:15:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-23 18:16:39]

Patchset 5 (id:??) landed as
https://crrev.com/27f39f26c1636f9f09c79ab44f86e91b114b9a61
Cr-Commit-Position: refs/heads/master@{#312877}