Move Android to use new Popup Blocker API.

Move Android to use new Popup Blocker API.

Update Android's popup implementation to use the new popup blocker API.

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

[David Trainor- moved to gerrit, 2013-08-20 22:37:47]

ptal.

yfriedman/tedchoc: General logic and all android bits
joth: web_contents_delegate_android (small change, we weren't setting the
blocked variable if we didn't block the tab).
jochen: popup_blocker_tab_helper stuff, chrome/*, general approval.

[joth, 2013-08-20 22:44:36]

https://codereview.chromium.org/23264020/diff/8001/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/23264020/diff/8001/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.cc:138:
handled = Java_WebContentsDelegateAndroid_addNewContents(
note this method isn't used in webview (see
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...)
so it might be clearer for everyone to remove it from WCDA and put it straight
into ChromeWebContentsDelegate

pretty please :-)

https://codereview.chromium.org/23264020/diff/8001/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.cc:149:
*was_blocked = false;
seems like this should be !handled  instead of false?

[jochen (gone - plz use gerrit), 2013-08-21 08:05:25]

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:276: void
ChromeWebContentsDelegateAndroid::AddNewContents(
this isn't really related to popup blocking, but for instant search

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/tab_model/tab_model_list.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/tab_model/tab_model_list.cc:41:
tab->HandleNavigation(params);
does HandleNavigation() support params.should_set_opener?

I wonder whether there's a way to make the popup blocker browsertest run on
android

[David Trainor- moved to gerrit, 2013-08-21 17:25:12]

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/tab_model/tab_model_list.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/tab_model/tab_model_list.cc:41:
tab->HandleNavigation(params);
On 2013/08/21 08:05:25, jochen wrote:
> does HandleNavigation() support params.should_set_opener?
> 
> I wonder whether there's a way to make the popup blocker browsertest run on
> android

When the tab talks to the model in Java it can specify whether or not it wants
to be a parent, which can be based on should_set_opener (I'm making an
assumption on what this flag means based on the description if I'm wrong let me
know!).

We don't fully support NavigateParams yet, but I wanted to add basic support so
the Popups will work.

https://codereview.chromium.org/23264020/diff/8001/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/23264020/diff/8001/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.cc:138:
handled = Java_WebContentsDelegateAndroid_addNewContents(
On 2013/08/20 22:44:36, joth wrote:
> note this method isn't used in webview (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...)
> so it might be clearer for everyone to remove it from WCDA and put it straight
> into ChromeWebContentsDelegate
> 
> pretty please :-)

If you guys don't use it sure!

https://codereview.chromium.org/23264020/diff/8001/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.cc:149:
*was_blocked = false;
On 2013/08/20 22:44:36, joth wrote:
> seems like this should be !handled  instead of false?
> 

I wasn't sure if whether or not we handled it actually meant it was blocked or
not.  If we just don't show it it wasn't blocked?  I can remove this line for
now though and address it in another CL as it's kind of independent.

[David Trainor- moved to gerrit, 2013-08-21 17:27:03]

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:276: void
ChromeWebContentsDelegateAndroid::AddNewContents(
On 2013/08/21 08:05:25, jochen wrote:
> this isn't really related to popup blocking, but for instant search

We were using this code path downstream for popups as well as handling things
like instant (even though we don't use it for that reason).  I just moved it up
here and removed it from the popup pipeline so that it's all upstream.

[Yaron, 2013-08-21 19:07:58]

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:233: if (!source
|| (disposition != CURRENT_TAB &&
Can source be NULL? Won't we just crash later?

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:253: if (source)
You've already de-refed source on L243.

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:258: if
(popup_blocker_helper) {
No need for if after DCHECK

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:284:
DCHECK(disposition != SAVE_TO_DISK);
DCHECK_NE

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:286:
DCHECK(disposition != CURRENT_TAB);
DCHECK_NE

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:289: if (source)
Indentation off and this may be unnecessary?

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/tab_model/tab_model_list.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/tab_model/tab_model_list.cc:40: if (tab)
Hmm. How does this happen? We've create a new web contents and it's not
associated with a tab? From prerendering? I'm not saying it can't, I'd like to
understand it better though.

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/blocked_...
File chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/blocked_...
chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc:147:
TabModelList::HandleNavigation(&popup->params);
So maybe this is how you can clean up the browser_navigator.cc dependency. You
could have android also implement chrome::Navigate with a different cc-file and
avoid introducing depending on browser_navigator.cc

https://codereview.chromium.org/23264020/diff/8001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (left):

https://codereview.chromium.org/23264020/diff/8001/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:2829: 'browser/ui/browser_navigator.cc',
I think this is left over from your temporary work. You aren't using it now and
I'd prefer to keep browser_* out of android for consistency. Oh actually, I see
that you're using NavigatePrams from there. Hmm. I think we should probably
split NavigateParams that's profile-dependent out from the browser-dependent
variant or remove the Browser dep from it.

https://codereview.chromium.org/23264020/diff/8001/components/web_contents_de...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/23264020/diff/8001/components/web_contents_de...
components/web_contents_delegate_android/web_contents_delegate_android.cc:138:
handled = Java_WebContentsDelegateAndroid_addNewContents(
On 2013/08/20 22:44:36, joth wrote:
> note this method isn't used in webview (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...)
> so it might be clearer for everyone to remove it from WCDA and put it straight
> into ChromeWebContentsDelegate
> 
> pretty please :-)

+1

[Yaron, 2013-08-21 19:09:46]

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/tab_model/tab_model_list.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/tab_model/tab_model_list.cc:41:
tab->HandleNavigation(params);
On 2013/08/21 08:05:25, jochen wrote:
> does HandleNavigation() support params.should_set_opener?
> 
> I wonder whether there's a way to make the popup blocker browsertest run on
> android

Not really. chrome/ layer Browsertests rely on Browser which isn't used on
Android. It would be nice to have this abstracted some way but I think that's a
nontrivial amount of work

[David Trainor- moved to gerrit, 2013-08-21 20:21:27]

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:233: if (!source
|| (disposition != CURRENT_TAB &&
On 2013/08/21 19:07:58, Yaron wrote:
> Can source be NULL? Won't we just crash later?

The NULL check for source is done everywhere by browser, so I assumed it was a
valid case.

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:253: if (source)
On 2013/08/21 19:07:58, Yaron wrote:
> You've already de-refed source on L243.

Done.

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:258: if
(popup_blocker_helper) {
On 2013/08/21 19:07:58, Yaron wrote:
> No need for if after DCHECK

Done.

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:284:
DCHECK(disposition != SAVE_TO_DISK);
On 2013/08/21 19:07:58, Yaron wrote:
> DCHECK_NE

Done.

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:286:
DCHECK(disposition != CURRENT_TAB);
On 2013/08/21 19:07:58, Yaron wrote:
> DCHECK_NE

Done.

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:289: if (source)
On 2013/08/21 19:07:58, Yaron wrote:
> Indentation off and this may be unnecessary?

The code this was built off of checks for source both here and in
OpenURLFromTab.  I think it's a valid case to have a NULL source web contents.

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/tab_model/tab_model_list.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/tab_model/tab_model_list.cc:40: if (tab)
On 2013/08/21 19:07:58, Yaron wrote:
> Hmm. How does this happen? We've create a new web contents and it's not
> associated with a tab? From prerendering? I'm not saying it can't, I'd like to
> understand it better though.

The issue is popups can be blocked inside
ChromeContentBrowserClient::CanCreateWindow now.  The problem with this is that
it's moved out of ChromeWebContentsDelegateAndroid which used to pretty much
guarantee we had a TabAndroid (as that was the thing that registered the
delegate).  In this case I don't know that I can safely make that assumption...

I can add a DCHECK to catch any instances where we are dropping these calls on
the floor and we can hopefully catch them.  I would doubt that we'd see this
happen though.  Prerendering would be the only time I could see this being NULL,
but if we blocked and tried to show the popup here the user must have clicked on
the UI to allow popups through, which wouldn't show on a page that is being
prerendered I think.

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/blocked_...
File chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/blocked_...
chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc:147:
TabModelList::HandleNavigation(&popup->params);
On 2013/08/21 19:07:58, Yaron wrote:
> So maybe this is how you can clean up the browser_navigator.cc dependency. You
> could have android also implement chrome::Navigate with a different cc-file
and
> avoid introducing depending on browser_navigator.cc

To do that I'd have to pull apart browser_navigator.cc as right now
NavigateParams are in there along with these methods.  I can do that but I'd
rather do that in another CL.  Also chrome::Navigate does a LOT of stuff. 
Having a general case implementation will require more work.

https://codereview.chromium.org/23264020/diff/8001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (left):

https://codereview.chromium.org/23264020/diff/8001/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:2829: 'browser/ui/browser_navigator.cc',
I agree with what you're saying.  I think that's outside the scope of this
particular CL but it should be done.  I can file a bug and track that/throw it
on the pile of refactors I want to do :).

On 2013/08/21 19:07:58, Yaron wrote:
> I think this is left over from your temporary work. You aren't using it now
and
> I'd prefer to keep browser_* out of android for consistency. Oh actually, I
see
> that you're using NavigatePrams from there. Hmm. I think we should probably
> split NavigateParams that's profile-dependent out from the browser-dependent
> variant or remove the Browser dep from it.

[Ted C, 2013-08-21 21:06:14]

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:258: if
(popup_blocker_helper) {
On 2013/08/21 19:07:58, Yaron wrote:
> No need for if after DCHECK

Really?  I thought you had to fail gracefully for DCHECK, but this will just
crash down below now.

https://codereview.chromium.org/23264020/diff/22001/chrome/browser/android/ch...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/23264020/diff/22001/chrome/browser/android/ch...
chrome/browser/android/chrome_web_contents_delegate_android.cc:258:
params.disposition == NEW_FOREGROUND_TAB ||
should these be indented +1 to match with the opening paren above?  Just
wondering if it should be the same as the disposition conditional above.  No
really idea as it will probably look odd having these lines be indented just one
more than the rest.

[joth, 2013-08-21 21:12:00]

components/wcda lgtm but you'll need to fixup aw/ too

https://codereview.chromium.org/23264020/diff/22001/components/web_contents_d...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java
(left):

https://codereview.chromium.org/23264020/diff/22001/components/web_contents_d...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:62:
return false;
you'll need to remove this overload from the android_webview subclass. (It's
@Override so you should have a build error)

[David Trainor- moved to gerrit, 2013-08-21 21:18:27]

https://chromiumcodereview.appspot.com/23264020/diff/22001/chrome/browser/and...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/23264020/diff/22001/chrome/browser/and...
chrome/browser/android/chrome_web_contents_delegate_android.cc:258:
params.disposition == NEW_FOREGROUND_TAB ||
On 2013/08/21 21:06:15, Ted C wrote:
> should these be indented +1 to match with the opening paren above?  Just
> wondering if it should be the same as the disposition conditional above.  No
> really idea as it will probably look odd having these lines be indented just
one
> more than the rest.

Done.

[David Trainor- moved to gerrit, 2013-08-21 21:21:59]

On 2013/08/21 21:12:00, joth wrote:
> components/wcda lgtm but you'll need to fixup aw/ too
> 
>
https://codereview.chromium.org/23264020/diff/22001/components/web_contents_d...
> File
>
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java
> (left):
> 
>
https://codereview.chromium.org/23264020/diff/22001/components/web_contents_d...
>
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:62:
> return false;
> you'll need to remove this overload from the android_webview subclass. (It's
> @Override so you should have a build error)

There isn't an @Override on AwWebContentsDelegate#addNewContents.  You guys used
a different method that only takes 2 arguments instead of the one that takes 5.

[joth, 2013-08-21 21:56:24]

This one: AwWebContentsDelegateAdapter
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...



On 21 August 2013 14:21, <dtrainor@chromium.org> wrote:

> On 2013/08/21 21:12:00, joth wrote:
>
>> components/wcda lgtm but you'll need to fixup aw/ too
>>
>
>
> https://codereview.chromium.**org/23264020/diff/22001/**
> components/web_contents_**delegate_android/android/java/**
> src/org/chromium/components/**web_contents_delegate_android/**
>
WebContentsDelegateAndroid.**java<https://codereview.chromium.org/23264020/diff/22001/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java>
>
>> File
>>
>
> components/web_contents_**delegate_android/android/java/**
> src/org/chromium/components/**web_contents_delegate_android/**
> WebContentsDelegateAndroid.**java
>
>> (left):
>>
>
>
> https://codereview.chromium.**org/23264020/diff/22001/**
> components/web_contents_**delegate_android/android/java/**
> src/org/chromium/components/**web_contents_delegate_android/**
>
WebContentsDelegateAndroid.**java#oldcode62<https://codereview.chromium.org/23264020/diff/22001/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java#oldcode62>
>
> components/web_contents_**delegate_android/android/java/**
> src/org/chromium/components/**web_contents_delegate_android/**
> WebContentsDelegateAndroid.**java:62:
>
>> return false;
>> you'll need to remove this overload from the android_webview subclass.
>> (It's
>> @Override so you should have a build error)
>>
>
> There isn't an @Override on AwWebContentsDelegate#**addNewContents.  You
> guys used
> a different method that only takes 2 arguments instead of the one that
> takes 5.
>
>
https://chromiumcodereview.**appspot.com/23264020/<https://chromiumcodereview...
>

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

[joth, 2013-08-21 21:57:28]

See
http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...

external/chromium_org/android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:94:
method does not override or implement a method from a supertype
    @Override
    ^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
1 errormake: ***
[out/target/common/obj/JAVA_LIBRARIES/android_webview_java_intermediates/classes-full-debug.jar]
Error 41
make: *** Waiting for unfinished jobs....




On 21 August 2013 14:56, Jonathan Dixon <joth@chromium.org> wrote:

> This one: AwWebContentsDelegateAdapter
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
>
>
>
> On 21 August 2013 14:21, <dtrainor@chromium.org> wrote:
>
>> On 2013/08/21 21:12:00, joth wrote:
>>
>>> components/wcda lgtm but you'll need to fixup aw/ too
>>>
>>
>>
>> https://codereview.chromium.**org/23264020/diff/22001/**
>> components/web_contents_**delegate_android/android/java/**
>> src/org/chromium/components/**web_contents_delegate_android/**
>>
WebContentsDelegateAndroid.**java<https://codereview.chromium.org/23264020/diff/22001/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java>
>>
>>> File
>>>
>>
>> components/web_contents_**delegate_android/android/java/**
>> src/org/chromium/components/**web_contents_delegate_android/**
>> WebContentsDelegateAndroid.**java
>>
>>> (left):
>>>
>>
>>
>> https://codereview.chromium.**org/23264020/diff/22001/**
>> components/web_contents_**delegate_android/android/java/**
>> src/org/chromium/components/**web_contents_delegate_android/**
>>
WebContentsDelegateAndroid.**java#oldcode62<https://codereview.chromium.org/23264020/diff/22001/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java#oldcode62>
>>
>> components/web_contents_**delegate_android/android/java/**
>> src/org/chromium/components/**web_contents_delegate_android/**
>> WebContentsDelegateAndroid.**java:62:
>>
>>> return false;
>>> you'll need to remove this overload from the android_webview subclass.
>>> (It's
>>> @Override so you should have a build error)
>>>
>>
>> There isn't an @Override on AwWebContentsDelegate#**addNewContents.  You
>> guys used
>> a different method that only takes 2 arguments instead of the one that
>> takes 5.
>>
>>
https://chromiumcodereview.**appspot.com/23264020/<https://chromiumcodereview...
>>
>
>

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

[Yaron, 2013-08-22 00:36:30]

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
chrome/browser/android/chrome_web_contents_delegate_android.cc:258: if
(popup_blocker_helper) {
On 2013/08/21 21:06:15, Ted C wrote:
> On 2013/08/21 19:07:58, Yaron wrote:
> > No need for if after DCHECK
> 
> Really?  I thought you had to fail gracefully for DCHECK, but this will just
> crash down below now.
Nope. From
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
:
"Use DCHECK() or NOTREACHED() as assertions, e.g. to document pre- and
post-conditions. A DCHECK() means "this condition must always be true", not
"this condition is normally true, but perhaps not in exceptional cases."  A
consequence of this is that you should not handle DCHECK() failures, even if
failure would result in a crash. Attempting to handle a DCHECK() failure is a
statement that the DCHECK() can fail, which contradicts the point of writing the
DCHECK("

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
File chrome/browser/ui/android/tab_model/tab_model_list.cc (right):

https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
chrome/browser/ui/android/tab_model/tab_model_list.cc:40: if (tab)
On 2013/08/21 20:21:27, David Trainor wrote:
> On 2013/08/21 19:07:58, Yaron wrote:
> > Hmm. How does this happen? We've create a new web contents and it's not
> > associated with a tab? From prerendering? I'm not saying it can't, I'd like
to
> > understand it better though.
> 
> The issue is popups can be blocked inside
> ChromeContentBrowserClient::CanCreateWindow now.  The problem with this is
that
> it's moved out of ChromeWebContentsDelegateAndroid which used to pretty much
> guarantee we had a TabAndroid (as that was the thing that registered the
> delegate).  In this case I don't know that I can safely make that
assumption...
> 
> I can add a DCHECK to catch any instances where we are dropping these calls on
> the floor and we can hopefully catch them.  I would doubt that we'd see this
> happen though.  Prerendering would be the only time I could see this being
NULL,
> but if we blocked and tried to show the popup here the user must have clicked
on
> the UI to allow popups through, which wouldn't show on a page that is being
> prerendered I think.

Ok, so update the DCHECK with a message to contact dtrainor/yfriedman with more
information if this happens. Also, drop the "if" on the next line

https://codereview.chromium.org/23264020/diff/22001/chrome/browser/android/ta...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/23264020/diff/22001/chrome/browser/android/ta...
chrome/browser/android/tab_android.h:51: virtual void
HandleNavigation(chrome::NavigateParams* params) = 0;
I don't think TestShell will link. To this end, could we test popup blocker in
testshell after this change or does it also require
https://codereview.chromium.org/23020009/?

Also, it's a little strange that this is only used by blocked popup case (i.e.
someone looking at the code might expect all navigations to go through here). We
discussed this briefly offline and I think it'd be hard to get all navigations
to go through this one path. Maybe instead we should rename this so it's clear
it's for handling a navigation with a popup.

[David Trainor- moved to gerrit, 2013-08-22 00:41:56]

On 2013/08/21 21:57:28, joth wrote:
> See
>
http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...
> 
>
external/chromium_org/android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:94:
> method does not override or implement a method from a supertype
>     @Override
>     ^
> Note: Some input files use or override a deprecated API.
> Note: Recompile with -Xlint:deprecation for details.
> 1 errormake: ***
>
[out/target/common/obj/JAVA_LIBRARIES/android_webview_java_intermediates/classes-full-debug.jar]
> Error 41
> make: *** Waiting for unfinished jobs....
> 
> 
> 
> 
> On 21 August 2013 14:56, Jonathan Dixon <mailto:joth@chromium.org> wrote:
> 
> > This one: AwWebContentsDelegateAdapter
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
> >
> >
> >
> > On 21 August 2013 14:21, <mailto:dtrainor@chromium.org> wrote:
> >
> >> On 2013/08/21 21:12:00, joth wrote:
> >>
> >>> components/wcda lgtm but you'll need to fixup aw/ too
> >>>
> >>
> >>
> >> https://codereview.chromium.**org/23264020/diff/22001/**
> >> components/web_contents_**delegate_android/android/java/**
> >> src/org/chromium/components/**web_contents_delegate_android/**
> >>
>
WebContentsDelegateAndroid.**java<https://codereview.chromium.org/23264020/diff/22001/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java>
> >>
> >>> File
> >>>
> >>
> >> components/web_contents_**delegate_android/android/java/**
> >> src/org/chromium/components/**web_contents_delegate_android/**
> >> WebContentsDelegateAndroid.**java
> >>
> >>> (left):
> >>>
> >>
> >>
> >> https://codereview.chromium.**org/23264020/diff/22001/**
> >> components/web_contents_**delegate_android/android/java/**
> >> src/org/chromium/components/**web_contents_delegate_android/**
> >>
>
WebContentsDelegateAndroid.**java#oldcode62<https://codereview.chromium.org/23264020/diff/22001/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java#oldcode62>
> >>
> >> components/web_contents_**delegate_android/android/java/**
> >> src/org/chromium/components/**web_contents_delegate_android/**
> >> WebContentsDelegateAndroid.**java:62:
> >>
> >>> return false;
> >>> you'll need to remove this overload from the android_webview subclass.
> >>> (It's
> >>> @Override so you should have a build error)
> >>>
> >>
> >> There isn't an @Override on AwWebContentsDelegate#**addNewContents.  You
> >> guys used
> >> a different method that only takes 2 arguments instead of the one that
> >> takes 5.
> >>
> >>
>
https://chromiumcodereview.**appspot.com/23264020/%3Chttps://chromiumcoderevi...>
> >>
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Oh I didn't see the adapter.  I'll fix that thanks!

[David Trainor- moved to gerrit, 2013-08-22 00:41:58]

On 2013/08/21 21:57:28, joth wrote:
> See
>
http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...
> 
>
external/chromium_org/android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:94:
> method does not override or implement a method from a supertype
>     @Override
>     ^
> Note: Some input files use or override a deprecated API.
> Note: Recompile with -Xlint:deprecation for details.
> 1 errormake: ***
>
[out/target/common/obj/JAVA_LIBRARIES/android_webview_java_intermediates/classes-full-debug.jar]
> Error 41
> make: *** Waiting for unfinished jobs....
> 
> 
> 
> 
> On 21 August 2013 14:56, Jonathan Dixon <mailto:joth@chromium.org> wrote:
> 
> > This one: AwWebContentsDelegateAdapter
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
> >
> >
> >
> > On 21 August 2013 14:21, <mailto:dtrainor@chromium.org> wrote:
> >
> >> On 2013/08/21 21:12:00, joth wrote:
> >>
> >>> components/wcda lgtm but you'll need to fixup aw/ too
> >>>
> >>
> >>
> >> https://codereview.chromium.**org/23264020/diff/22001/**
> >> components/web_contents_**delegate_android/android/java/**
> >> src/org/chromium/components/**web_contents_delegate_android/**
> >>
>
WebContentsDelegateAndroid.**java<https://codereview.chromium.org/23264020/diff/22001/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java>
> >>
> >>> File
> >>>
> >>
> >> components/web_contents_**delegate_android/android/java/**
> >> src/org/chromium/components/**web_contents_delegate_android/**
> >> WebContentsDelegateAndroid.**java
> >>
> >>> (left):
> >>>
> >>
> >>
> >> https://codereview.chromium.**org/23264020/diff/22001/**
> >> components/web_contents_**delegate_android/android/java/**
> >> src/org/chromium/components/**web_contents_delegate_android/**
> >>
>
WebContentsDelegateAndroid.**java#oldcode62<https://codereview.chromium.org/23264020/diff/22001/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java#oldcode62>
> >>
> >> components/web_contents_**delegate_android/android/java/**
> >> src/org/chromium/components/**web_contents_delegate_android/**
> >> WebContentsDelegateAndroid.**java:62:
> >>
> >>> return false;
> >>> you'll need to remove this overload from the android_webview subclass.
> >>> (It's
> >>> @Override so you should have a build error)
> >>>
> >>
> >> There isn't an @Override on AwWebContentsDelegate#**addNewContents.  You
> >> guys used
> >> a different method that only takes 2 arguments instead of the one that
> >> takes 5.
> >>
> >>
>
https://chromiumcodereview.**appspot.com/23264020/%3Chttps://chromiumcoderevi...>
> >>
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Oh I didn't see the adapter.  I'll fix that thanks!

[David Trainor- moved to gerrit, 2013-08-22 00:42:40]

On 2013/08/22 00:36:30, Yaron wrote:
>
https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
> File chrome/browser/android/chrome_web_contents_delegate_android.cc (right):
> 
>
https://codereview.chromium.org/23264020/diff/8001/chrome/browser/android/chr...
> chrome/browser/android/chrome_web_contents_delegate_android.cc:258: if
> (popup_blocker_helper) {
> On 2013/08/21 21:06:15, Ted C wrote:
> > On 2013/08/21 19:07:58, Yaron wrote:
> > > No need for if after DCHECK
> > 
> > Really?  I thought you had to fail gracefully for DCHECK, but this will just
> > crash down below now.
> Nope. From
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> :
> "Use DCHECK() or NOTREACHED() as assertions, e.g. to document pre- and
> post-conditions. A DCHECK() means "this condition must always be true", not
> "this condition is normally true, but perhaps not in exceptional cases."
 A
> consequence of this is that you should not handle DCHECK() failures, even if
> failure would result in a crash. Attempting to handle a DCHECK() failure is a
> statement that the DCHECK() can fail, which contradicts the point of writing
the
> DCHECK("
> 
>
https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
> File chrome/browser/ui/android/tab_model/tab_model_list.cc (right):
> 
>
https://codereview.chromium.org/23264020/diff/8001/chrome/browser/ui/android/...
> chrome/browser/ui/android/tab_model/tab_model_list.cc:40: if (tab)
> On 2013/08/21 20:21:27, David Trainor wrote:
> > On 2013/08/21 19:07:58, Yaron wrote:
> > > Hmm. How does this happen? We've create a new web contents and it's not
> > > associated with a tab? From prerendering? I'm not saying it can't, I'd
like
> to
> > > understand it better though.
> > 
> > The issue is popups can be blocked inside
> > ChromeContentBrowserClient::CanCreateWindow now.  The problem with this is
> that
> > it's moved out of ChromeWebContentsDelegateAndroid which used to pretty much
> > guarantee we had a TabAndroid (as that was the thing that registered the
> > delegate).  In this case I don't know that I can safely make that
> assumption...
> > 
> > I can add a DCHECK to catch any instances where we are dropping these calls
on
> > the floor and we can hopefully catch them.  I would doubt that we'd see this
> > happen though.  Prerendering would be the only time I could see this being
> NULL,
> > but if we blocked and tried to show the popup here the user must have
clicked
> on
> > the UI to allow popups through, which wouldn't show on a page that is being
> > prerendered I think.
> 
> Ok, so update the DCHECK with a message to contact dtrainor/yfriedman with
more
> information if this happens. Also, drop the "if" on the next line
> 
>
https://codereview.chromium.org/23264020/diff/22001/chrome/browser/android/ta...
> File chrome/browser/android/tab_android.h (right):
> 
>
https://codereview.chromium.org/23264020/diff/22001/chrome/browser/android/ta...
> chrome/browser/android/tab_android.h:51: virtual void
> HandleNavigation(chrome::NavigateParams* params) = 0;
> I don't think TestShell will link. To this end, could we test popup blocker in
> testshell after this change or does it also require
> https://codereview.chromium.org/23020009/?
> 
> Also, it's a little strange that this is only used by blocked popup case (i.e.
> someone looking at the code might expect all navigations to go through here).
We
> discussed this briefly offline and I think it'd be hard to get all navigations
> to go through this one path. Maybe instead we should rename this so it's clear
> it's for handling a navigation with a popup.

Makes sense.  I'll add a comment, remove the if, and rename the method.

[jochen (gone - plz use gerrit), 2013-08-22 06:31:26]

lgtm

[David Trainor- moved to gerrit, 2013-08-22 18:22:28]

Fixed the testshell build break.

[joth, 2013-08-22 18:24:52]

aw/ lgtm

[Yaron, 2013-08-22 18:30:04]

lgtm

https://chromiumcodereview.appspot.com/23264020/diff/32001/chrome/android/tes...
File chrome/android/testshell/testshell_tab.cc (right):

https://chromiumcodereview.appspot.com/23264020/diff/32001/chrome/android/tes...
chrome/android/testshell/testshell_tab.cc:101: void
TestShellTab::HandlePopupNavigation(chrome::NavigateParams* params) { }
NOTIMPLEMENTED

[David Trainor- moved to gerrit, 2013-08-22 21:07:50]

On 2013/08/22 18:30:04, Yaron wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/23264020/diff/32001/chrome/android/tes...
> File chrome/android/testshell/testshell_tab.cc (right):
> 
>
https://chromiumcodereview.appspot.com/23264020/diff/32001/chrome/android/tes...
> chrome/android/testshell/testshell_tab.cc:101: void
> TestShellTab::HandlePopupNavigation(chrome::NavigateParams* params) { }
> NOTIMPLEMENTED

I thought about it but I don't want it to crash.  I'll add it for now though.

[joth, 2013-08-22 22:27:25]

FYI,  NOTIMPLEMENTED logs, it's NOTREACHED that crashes.


On 22 August 2013 14:07, <dtrainor@chromium.org> wrote:

> On 2013/08/22 18:30:04, Yaron wrote:
>
>> lgtm
>>
>
>
> https://chromiumcodereview.**appspot.com/23264020/diff/**
>
32001/chrome/android/**testshell/testshell_tab.cc<https://chromiumcodereview.appspot.com/23264020/diff/32001/chrome/android/testshell/testshell_tab.cc>
>
>> File chrome/android/testshell/**testshell_tab.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/23264020/diff/**
>
32001/chrome/android/**testshell/testshell_tab.cc#**newcode101<https://chromiumcodereview.appspot.com/23264020/diff/32001/chrome/android/testshell/testshell_tab.cc#newcode101>
>
>> chrome/android/testshell/**testshell_tab.cc:101: void
>> TestShellTab::**HandlePopupNavigation(chrome::**NavigateParams* params)
>> { }
>> NOTIMPLEMENTED
>>
>
> I thought about it but I don't want it to crash.  I'll add it for now
> though.
>
>
https://chromiumcodereview.**appspot.com/23264020/<https://chromiumcodereview...
>

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

[David Trainor- moved to gerrit, 2013-08-23 00:37:02]

On 2013/08/22 22:27:25, joth wrote:
> FYI,  NOTIMPLEMENTED logs, it's NOTREACHED that crashes.
> 
> 
> On 22 August 2013 14:07, <mailto:dtrainor@chromium.org> wrote:
> 
> > On 2013/08/22 18:30:04, Yaron wrote:
> >
> >> lgtm
> >>
> >
> >
> > https://chromiumcodereview.**appspot.com/23264020/diff/**
> >
>
32001/chrome/android/**testshell/testshell_tab.cc<https://chromiumcodereview.appspot.com/23264020/diff/32001/chrome/android/testshell/testshell_tab.cc>
> >
> >> File chrome/android/testshell/**testshell_tab.cc (right):
> >>
> >
> >
> > https://chromiumcodereview.**appspot.com/23264020/diff/**
> >
>
32001/chrome/android/**testshell/testshell_tab.cc#**newcode101<https://chromiumcodereview.appspot.com/23264020/diff/32001/chrome/android/testshell/testshell_tab.cc#newcode101>
> >
> >> chrome/android/testshell/**testshell_tab.cc:101: void
> >> TestShellTab::**HandlePopupNavigation(chrome::**NavigateParams* params)
> >> { }
> >> NOTIMPLEMENTED
> >>
> >
> > I thought about it but I don't want it to crash.  I'll add it for now
> > though.
> >
> >
>
https://chromiumcodereview.**appspot.com/23264020/%3Chttps://chromiumcoderevi...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ah yay.  Nevermind thanks! :)

[commit-bot: I haz the power, 2013-08-23 17:51:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/23264020/17012

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

Change committed as 219324