Browser plugin: Implement loadStart and loadAbort events

Browser plugin: Implement loadStart and loadAbort events

BUG=153534
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160205

[Fady Samuel, 2012-10-02 19:58:51]

Hi Charlie,

I've implemented loadStart and loadAbort but I'm uncertain how to test
loadAbort. Suggestions are welcome :-)

Thanks,

Fady

[Charlie Reis, 2012-10-03 01:52:30]

You can test loadAbort using URLs that testserver.py understands.  For example,
you can do a download with /download-known-size or a network error with
something like /close-socket.  Look for examples in other tests, since I can't
remember if the leading slash has to be present or omitted.

https://codereview.chromium.org/10941042/diff/20001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/10941042/diff/20001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:832:
test_guest->WaitForUpdateRectMsg();
There's a lot of boilerplate code here.  Is this mostly common across multiple
tests?  Is there a way to share it?

https://codereview.chromium.org/10941042/diff/20001/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/10941042/diff/20001/content/common/browser_pl...
content/common/browser_plugin_messages.h:135:
IPC_MESSAGE_CONTROL4(BrowserPluginMsg_LoadAbort,
This should have a comment describing the types, etc.

Also, I don't see anyone sending this message.  (Guess we do need a test for
it.)  :)

https://codereview.chromium.org/10941042/diff/20001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/10941042/diff/20001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:354: const std::string
kIsTopLevel = "isTopLevel";
It might make sense to move these to the top, since they're constants and used
in multiple places.

https://codereview.chromium.org/10941042/diff/20001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:387: const std::string kType =
"type";
Ditto, even for kType.

https://codereview.chromium.org/10941042/diff/20001/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/10941042/diff/20001/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:2: var url;
nit: loadStartUrl
That will make the test more obvious, and leave this flexible for other event
listeners.

https://codereview.chromium.org/10941042/diff/20001/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:28: 
nit: Remove.  (No need for extra code churn.)

[Fady Samuel, 2012-10-04 00:43:38]

https://codereview.chromium.org/10941042/diff/20001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/10941042/diff/20001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:832:
test_guest->WaitForUpdateRectMsg();
On 2012/10/03 01:52:30, creis wrote:
> There's a lot of boilerplate code here.  Is this mostly common across multiple
> tests?  Is there a way to share it?

Done.

https://codereview.chromium.org/10941042/diff/20001/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/10941042/diff/20001/content/common/browser_pl...
content/common/browser_plugin_messages.h:135:
IPC_MESSAGE_CONTROL4(BrowserPluginMsg_LoadAbort,
On 2012/10/03 01:52:30, creis wrote:
> This should have a comment describing the types, etc.
> 
> Also, I don't see anyone sending this message.  (Guess we do need a test for
> it.)  :)

Done. Although, I don't know if this is the best possible comment here, please
advise whether I should write something more descriptive. Thanks. I don't
exactly know what the cases are for a load to fail.

https://codereview.chromium.org/10941042/diff/20001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/10941042/diff/20001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:354: const std::string
kIsTopLevel = "isTopLevel";
On 2012/10/03 01:52:30, creis wrote:
> It might make sense to move these to the top, since they're constants and used
> in multiple places.

Done.

https://codereview.chromium.org/10941042/diff/20001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:387: const std::string kType =
"type";
On 2012/10/03 01:52:30, creis wrote:
> Ditto, even for kType.

Done.

https://codereview.chromium.org/10941042/diff/20001/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/10941042/diff/20001/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:2: var url;
On 2012/10/03 01:52:30, creis wrote:
> nit: loadStartUrl
> That will make the test more obvious, and leave this flexible for other event
> listeners.

Done.

https://codereview.chromium.org/10941042/diff/20001/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:28: 
On 2012/10/03 01:52:30, creis wrote:
> nit: Remove.  (No need for extra code churn.)

Done.

[Charlie Reis, 2012-10-04 02:57:48]

https://codereview.chromium.org/10941042/diff/27001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10941042/diff/27001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:80: // TODO(fsamuel):
content_shell seems to ignore this.
nit: Also have a comment saying why we return false, before the TODO.  (We
disable downloads in guests for now, but we will later expose API to allow
embedders to handle them.)

https://codereview.chromium.org/10941042/diff/27001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:306:
UTF16ToASCII(error_description)));
I was expecting us to return one of "networkError, download, canceled, sslError,
safeBrowsingError", but this looks like it would return a possibly empty
localized string.  That won't work well for developers trying to decide how to
handle an error.

My list of known values can be changed if needed, but I think we need something
that developers can check to tell if it failed because it was a download,
network error, etc.  Can the error_code tell us that, so that we can decide what
string to use in |type|?

https://codereview.chromium.org/10941042/diff/27001/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/10941042/diff/27001/content/common/browser_pl...
content/common/browser_plugin_messages.h:137: // of the error will be stored in
type.
I think we want this to be one of a set of known error types, so that developers
can compare against it.
nit: |type|

https://codereview.chromium.org/10941042/diff/27001/content/test/data/browser...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/10941042/diff/27001/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:3: document.title = 'abort';
Maybe we should set it to evt.type, so we can test that value?

https://codereview.chromium.org/10941042/diff/27001/content/test/data/browser...
content/test/data/browser_plugin_embedder.html:11:
plugin.addEventListener('loadAbort', loadAbort);
This will cause these listeners to be added multiple times in tests that call
SetSrc more than once.  Maybe we should have a separate function that installs
them onload?

[Fady Samuel, 2012-10-04 15:04:51]

http://codereview.chromium.org/10941042/diff/27001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

http://codereview.chromium.org/10941042/diff/27001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:80: // TODO(fsamuel):
content_shell seems to ignore this.
On 2012/10/04 02:57:48, creis wrote:
> nit: Also have a comment saying why we return false, before the TODO.  (We
> disable downloads in guests for now, but we will later expose API to allow
> embedders to handle them.)

Done.

http://codereview.chromium.org/10941042/diff/27001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:306:
UTF16ToASCII(error_description)));
On 2012/10/04 02:57:48, creis wrote:
> I was expecting us to return one of "networkError, download, canceled,
sslError,
> safeBrowsingError", but this looks like it would return a possibly empty
> localized string.  That won't work well for developers trying to decide how to
> handle an error.
> 
> My list of known values can be changed if needed, but I think we need
something
> that developers can check to tell if it failed because it was a download,
> network error, etc.  Can the error_code tell us that, so that we can decide
what
> string to use in |type|?

Done. Using net::ErrorToString. The list of errors can be found at:
net/base/net_error_list.h

http://codereview.chromium.org/10941042/diff/27001/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

http://codereview.chromium.org/10941042/diff/27001/content/common/browser_plu...
content/common/browser_plugin_messages.h:137: // of the error will be stored in
type.
On 2012/10/04 02:57:48, creis wrote:
> I think we want this to be one of a set of known error types, so that
developers
> can compare against it.
> nit: |type|

Done.

http://codereview.chromium.org/10941042/diff/27001/content/test/data/browser_...
File content/test/data/browser_plugin_embedder.html (right):

http://codereview.chromium.org/10941042/diff/27001/content/test/data/browser_...
content/test/data/browser_plugin_embedder.html:3: document.title = 'abort';
On 2012/10/04 02:57:48, creis wrote:
> Maybe we should set it to evt.type, so we can test that value?

Done.

http://codereview.chromium.org/10941042/diff/27001/content/test/data/browser_...
content/test/data/browser_plugin_embedder.html:11:
plugin.addEventListener('loadAbort', loadAbort);
On 2012/10/04 02:57:48, creis wrote:
> This will cause these listeners to be added multiple times in tests that call
> SetSrc more than once.  Maybe we should have a separate function that installs
> them onload?

Done.

[Charlie Reis, 2012-10-04 17:06:55]

Thanks.  I think we're all set, apart from a question about what error codes to
return.  Mihai, can we get your opinion?

https://codereview.chromium.org/10941042/diff/27001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10941042/diff/27001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:306:
UTF16ToASCII(error_description)));
On 2012/10/04 15:04:51, Fady Samuel wrote:
> On 2012/10/04 02:57:48, creis wrote:
> > I was expecting us to return one of "networkError, download, canceled,
> sslError,
> > safeBrowsingError", but this looks like it would return a possibly empty
> > localized string.  That won't work well for developers trying to decide how
to
> > handle an error.
> > 
> > My list of known values can be changed if needed, but I think we need
> something
> > that developers can check to tell if it failed because it was a download,
> > network error, etc.  Can the error_code tell us that, so that we can decide
> what
> > string to use in |type|?
> 
> Done. Using net::ErrorToString. The list of errors can be found at:
> net/base/net_error_list.h

Interesting...  I wonder if this is an appropriate list of error codes to expose
in an API, or if we should have a map of known strings to return, considering
most of these as "networkError".  Mihai, do you have any thoughts on this?

https://codereview.chromium.org/10941042/diff/34002/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10941042/diff/34002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:82: // TODO(fsamuel): We
disable downloads in guest sfor now, but we will later
nit: guests for

[Charlie Reis, 2012-10-04 17:51:38]

https://codereview.chromium.org/10941042/diff/34002/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/10941042/diff/34002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:614:
StartBrowserPluginTest(kEmbedderURL, "blank", true, "");
"blank" isn't a valid URL.  Did you mean "about:blank"?

[Mihai Parparita -not on Chrome, 2012-10-04 18:01:27]

On Thu, Oct 4, 2012 at 10:06 AM, <creis@chromium.org> wrote:

> Thanks.  I think we're all set, apart from a question about what error
> codes to
> return.  Mihai, can we get your opinion?


The webRequest API reports errors (
http://developer.chrome.com/extensions/webRequest.html#event-onErrorOccurred)
as strings, but has this warning "This string is not guaranteed to remain
backwards compatible between releases. You must not parse and act based
upon its content."

It ends up using net:ErrorToString (
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...
)

I think that approach should work here too.

Mihai

[Charlie Reis, 2012-10-04 18:04:41]

Awesome.  LGTM, once the two nits I mentioned are fixed.

[Fady Samuel, 2012-10-04 18:13:48]

Fixed nits. Committing now. Thanks for the confirmation, Mihai.

https://codereview.chromium.org/10941042/diff/34002/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10941042/diff/34002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:82: // TODO(fsamuel): We
disable downloads in guest sfor now, but we will later
On 2012/10/04 17:06:55, creis wrote:
> nit: guests for

Done.

https://codereview.chromium.org/10941042/diff/34002/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/10941042/diff/34002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:614:
StartBrowserPluginTest(kEmbedderURL, "blank", true, "");
On 2012/10/04 17:51:38, creis wrote:
> "blank" isn't a valid URL.  Did you mean "about:blank"?

Done.

[commit-bot: I haz the power, 2012-10-04 18:14:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10941042/33013

[commit-bot: I haz the power, 2012-10-04 18:17:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10941042/35005

[commit-bot: I haz the power, 2012-10-04 20:01:21]

Change committed as 160205