Replace merge-ui-and-compositor-threads flag with no-merge-ui-and-compositor-threads flag

Made the following changes to enable merge mode

- only set EnableWebViewSynchronousAPIs in unmerge mode
- Allow wait in UI thread
- Disable 8 unit tests since SW renderer didn't work.


BUG=235702, 239144, 238735
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199128

[joth, 2013-04-26 18:06:44]

https://codereview.chromium.org/13907007/diff/6001/android_webview/common/aw_...
File android_webview/common/aw_switches.h (right):

https://codereview.chromium.org/13907007/diff/6001/android_webview/common/aw_...
android_webview/common/aw_switches.h:10: // Not merge the Browser UI and the
renderer compositor threads.
nit: "Do not merge..."

https://codereview.chromium.org/13907007/diff/6001/android_webview/test/shell...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/13907007/diff/6001/android_webview/test/shell...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:34:
}
The plan was not to add this here to get upstream running in the new mode...
although I'm OK we land this patch as it is and do a follow up to remove this
again.

I think we'll need to add a couple more "if (no-merge-thread)" around some other
places in the code (e.g. where we set the kEnableWebViewSynchronousAPI flag - we
shouldn't be doing that in merged thread mode).

[michaelbai, 2013-04-26 21:41:43]

PTAL

https://codereview.chromium.org/13907007/diff/6001/android_webview/common/aw_...
File android_webview/common/aw_switches.h (right):

https://codereview.chromium.org/13907007/diff/6001/android_webview/common/aw_...
android_webview/common/aw_switches.h:10: // Not merge the Browser UI and the
renderer compositor threads.
On 2013/04/26 18:06:44, joth wrote:
> nit: "Do not merge..."

Done.

https://codereview.chromium.org/13907007/diff/6001/android_webview/test/shell...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/13907007/diff/6001/android_webview/test/shell...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:34:
}
We want to run downstream in no-merge mode and upstream in merge mode, there has
to be some difference between downstream and upstream. I were thinking the
difference is the setting in WebViewChromiumFactoryProvider.java, It seemed that
you also want to run these tests (android_webview_test) in upstream merge mode,
then where is the difference you think? The command line file?


On 2013/04/26 18:06:44, joth wrote:
> The plan was not to add this here to get upstream running in the new mode...
> although I'm OK we land this patch as it is and do a follow up to remove this
> again.
> 
> I think we'll need to add a couple more "if (no-merge-thread)" around some
other
> places in the code (e.g. where we set the kEnableWebViewSynchronousAPI flag -
we
> shouldn't be doing that in merged thread mode).

[joth, 2013-04-26 21:52:35]

https://codereview.chromium.org/13907007/diff/6001/android_webview/test/shell...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/13907007/diff/6001/android_webview/test/shell...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:34:
}

On 2013/04/26 21:41:43, michaelbai wrote:
> We want to run downstream in no-merge mode and upstream in merge mode, there
has
> to be some difference between downstream and upstream. I were thinking the
> difference is the setting in WebViewChromiumFactoryProvider.java, It seemed
that
> you also want to run these tests (android_webview_test) in upstream merge
mode,
> then where is the difference you think? The command line file?
> 

(Note this file is not used in downstream)
The difference is, down stream has these three lines (disable merge mode unless
the developer has specifically opted into it). Upstream won't (leaves code under
test in merge thread mode, unless developer has specifically opted out)

[michaelbai, 2013-04-27 04:04:04]

The tests still failed when disable kEnableWebViewSynchronousAPIs,  any other
flag should I look for?

[boliu, 2013-04-27 04:17:10]

These are DCHECKs on thread_restriction disallow wait on the UI thread. If you
run the test locally and symbolize the stack, I suspect the issue is inside gpu
initialization which sends a lot of synchronous IPCs. This is fine on the
renderer impl thread, but not on browser UI thread. I think these are OK in
practice since I didn't see any problems (over a month ago when I coded this up)

The hack workaround (not landable) is here:
https://codereview.chromium.org/12356002/diff/7003/content/browser/browser_ma...

So what you need to do is create a ScopedAllowWaitForLegacyWebViewApi on the
heap, from android webview specific code that's run after CreateThreads.
Probably some method of AwBrowserMainParts should be fine.

[commit-bot: I haz the power, 2013-05-02 04:21:44]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[michaelbai, 2013-05-07 22:36:52]

PTAL, don't know why the test bots didn't show up

[michaelbai, 2013-05-08 03:35:02]

On 2013/05/07 22:36:52, michaelbai wrote:
> PTAL, don't know why the test bots didn't show up

Shall I disable the other failed 6 tests also?

[boliu, 2013-05-08 08:02:49]

On 2013/05/08 03:35:02, michaelbai wrote:
> On 2013/05/07 22:36:52, michaelbai wrote:
> > PTAL, don't know why the test bots didn't show up
> 
> Shall I disable the other failed 6 tests also?

+mnaganov

All these looks viewport zoom related, not too unexpected I guess. They are
timeouts though. Can we get some information if it's because it's waiting for a
callback that never happens, or if it's waiting for a particular value that's
not set?

[mnaganov (inactive), 2013-05-08 08:12:13]

On 2013/05/08 08:02:49, boliu wrote:
> On 2013/05/08 03:35:02, michaelbai wrote:
> > On 2013/05/07 22:36:52, michaelbai wrote:
> > > PTAL, don't know why the test bots didn't show up
> > 
> > Shall I disable the other failed 6 tests also?
> 
> +mnaganov
> 
> All these looks viewport zoom related, not too unexpected I guess. They are
> timeouts though. Can we get some information if it's because it's waiting for
a
> callback that never happens, or if it's waiting for a particular value that's
> not set?

All these timeouts is due to OnScaleChanged callback never gets called in
situations where scale is expected to change.

[mkosiba (inactive), 2013-05-08 10:42:27]

ah, this is probably related to the FrameMetadata not being delivered. IIRC
Alex was suggesting we add a separate callback for this instead of reusing
the metadata.


On Wed, May 8, 2013 at 9:12 AM, <mnaganov@chromium.org> wrote:

> On 2013/05/08 08:02:49, boliu wrote:
>
>> On 2013/05/08 03:35:02, michaelbai wrote:
>> > On 2013/05/07 22:36:52, michaelbai wrote:
>> > > PTAL, don't know why the test bots didn't show up
>> >
>> > Shall I disable the other failed 6 tests also?
>>
>
>  +mnaganov
>>
>
>  All these looks viewport zoom related, not too unexpected I guess. They
>> are
>> timeouts though. Can we get some information if it's because it's waiting
>> for
>>
> a
>
>> callback that never happens, or if it's waiting for a particular value
>> that's
>> not set?
>>
>
> All these timeouts is due to OnScaleChanged callback never gets called in
> situations where scale is expected to change.
>
>
https://codereview.chromium.**org/13907007/<https://codereview.chromium.org/1...
>

[boliu, 2013-05-08 10:58:39]

On 2013/05/08 10:42:27, mkosiba wrote:
> ah, this is probably related to the FrameMetadata not being delivered. IIRC
> Alex was suggesting we add a separate callback for this instead of reusing
> the metadata.

I vote for file bug, disable these tests, and land this.

But should wait for we push another version to master first though, which should
happen soon-ish. I guess keep spamming android bots until then.

Also update the description, and add 238735 and new bug to bug list.

[boliu, 2013-05-08 12:46:02]

Joth just reminded me that this will not affect downstream. So let's land this
asap.

https://codereview.chromium.org/13907007/diff/45001/android_webview/lib/main/...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/13907007/diff/45001/android_webview/lib/main/...
android_webview/lib/main/aw_main_delegate.cc:79: if
(!UIAndRendererCompositorThreadsNotMerged()) {
Leave a comment that this is temporary until we remove the browser compositor

[joth, 2013-05-08 12:49:11]

lgtm

yeah The various negatives in function names gets hard to read, but this stuff
is not going to remain for that long so we can live with it for now.

Thanks

https://codereview.chromium.org/13907007/diff/45001/android_webview/lib/main/...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/13907007/diff/45001/android_webview/lib/main/...
android_webview/lib/main/aw_main_delegate.cc:79: if
(!UIAndRendererCompositorThreadsNotMerged()) {
On 2013/05/08 12:46:03, boliu wrote:
> Leave a comment that this is temporary until we remove the browser compositor

+1

[commit-bot: I haz the power, 2013-05-08 19:34:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/13907007/60001

[boliu, 2013-05-08 19:36:53]

Err!! Description!! Title and description is not the same thing. Right now it
only contains BUG=...

Need to unselect cq, update the descritpion, wait for ~10 minutes, then select
cq again

[commit-bot: I haz the power, 2013-05-08 23:23:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/13907007/78001

[commit-bot: I haz the power, 2013-05-09 05:39:53]

Change committed as 199128