Add WebUIDataSourceImpl::DisableReplaceExistingSource

Add WebUIDataSourceImpl::DisableReplaceExistingSource

Currently when using WebUIDataSource object returned by content::WebUIDataSource::Create,
there is no way to override ShouldReplaceExistingSource for the URLDataSource object that
will be created by WebUIDataSource internally. As a result when WebUIDataSource of the same
type is added repeatedly for different instances of WebUI, the previous pending request on the data
source will be cancelled, resulting in broken WebUI page.
The New Tab Page in chromium doesn't suffer because it doesn't use WebUIDataSource,
however it's convinient and less work to use it for some WebUIs. It's also possible to work around
by avoiding adding WebUIDataSource repeatedly in the first place, but that is much more complicated
as we'll have to track is a certain WebUIDataSource already added or not for a given profile.
The proposed fix simply add the api to override ShouldReplaceExistingSource.

BUG=

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

[shuais_, 2013-08-05 07:38:36]

Hi guys, please have a look at this small patch.

[Evan Stade, 2013-08-05 20:50:55]

nothing actually calls DisableReplaceExistingSource(), so this is dead code.

[shuais_, 2013-08-06 02:58:32]

On 2013/08/05 20:50:55, Evan Stade wrote:
> nothing actually calls DisableReplaceExistingSource(), so this is dead code.

We do use it in Opera though.. maybe I should accompany it with a test?

[arv (Not doing code reviews), 2013-08-07 13:44:18]

A test or a comment would be good. Currently, it is possible that someone will
delete it after figuring out that it is not used anywhere.

[Evan Stade, 2013-08-07 23:12:02]

On 2013/08/06 02:58:32, shuais_ wrote:
> On 2013/08/05 20:50:55, Evan Stade wrote:
> > nothing actually calls DisableReplaceExistingSource(), so this is dead code.
> 
> We do use it in Opera though.. maybe I should accompany it with a test?

I'm not familiar with the upstreaming plans for Opera, is there a document
somewhere you can link me to?

In general, code that's only needed downstream should remain downstream.

[Evan Stade, 2013-08-07 23:18:19]

+jam for content api change.

[shuais_, 2013-08-08 09:53:06]

On 2013/08/07 23:12:02, Evan Stade wrote:
> On 2013/08/06 02:58:32, shuais_ wrote:
> > On 2013/08/05 20:50:55, Evan Stade wrote:
> > > nothing actually calls DisableReplaceExistingSource(), so this is dead
code.
> > 
> > We do use it in Opera though.. maybe I should accompany it with a test?
> 
> I'm not familiar with the upstreaming plans for Opera, is there a document
> somewhere you can link me to?

Not sure what kind of document you were expecting? I think as a general rule we
try to 
upstream all changes that should reside in chromium, especially when it's
(potentially)
also beneficial for chromium.
 
> In general, code that's only needed downstream should remain downstream.

This patch may be only used by Opera for now, but it can be useful for any new
WebUI that
has mutiple instances. 

NTP doesn't use WebUIDataSource and implements URLDataSource directly, but it
could be
a (though small) improvement to switch to WebUIDataSource as well.

[jam, 2013-08-08 15:23:43]

lgtm, please add a comment in the header saying that this is used by embedders
so people know not to remove it

[Evan Stade, 2013-08-08 17:57:14]

On 2013/08/08 09:53:06, shuais_ wrote:
> On 2013/08/07 23:12:02, Evan Stade wrote:
> > On 2013/08/06 02:58:32, shuais_ wrote:
> > > On 2013/08/05 20:50:55, Evan Stade wrote:
> > > > nothing actually calls DisableReplaceExistingSource(), so this is dead
> code.
> > > 
> > > We do use it in Opera though.. maybe I should accompany it with a test?
> > 
> > I'm not familiar with the upstreaming plans for Opera, is there a document
> > somewhere you can link me to?
> 
> Not sure what kind of document you were expecting? I think as a general rule
we
> try to 
> upstream all changes that should reside in chromium, especially when it's
> (potentially)
> also beneficial for chromium.

What prevents this code from being broken or deleted by chromium authors who
don't have any visiblity into how it's actually used? What enforces that the
code is removed when embedders no longer need it?

>  
> > In general, code that's only needed downstream should remain downstream.
> 
> This patch may be only used by Opera for now, but it can be useful for any new
> WebUI that
> has mutiple instances. 
> 
> NTP doesn't use WebUIDataSource and implements URLDataSource directly, but it
> could be
> a (though small) improvement to switch to WebUIDataSource as well.

If it can be useful for chromium, please update chromium so that a) chromium
gets the benefit b) everyone can see that it's useful c) I can understand
whether this patch is correct and necessary.

[shuais_, 2013-08-12 08:12:42]

On 2013/08/08 17:57:14, Evan Stade wrote:
> On 2013/08/08 09:53:06, shuais_ wrote:
> > On 2013/08/07 23:12:02, Evan Stade wrote:
> > > On 2013/08/06 02:58:32, shuais_ wrote:
> > > > On 2013/08/05 20:50:55, Evan Stade wrote:
> > > > > nothing actually calls DisableReplaceExistingSource(), so this is dead
> > code.
> > > > 
> > > > We do use it in Opera though.. maybe I should accompany it with a test?
> > > 
> > > I'm not familiar with the upstreaming plans for Opera, is there a document
> > > somewhere you can link me to?
> > 
> > Not sure what kind of document you were expecting? I think as a general rule
> we
> > try to 
> > upstream all changes that should reside in chromium, especially when it's
> > (potentially)
> > also beneficial for chromium.
> 
> What prevents this code from being broken or deleted by chromium authors who
> don't have any visiblity into how it's actually used? What enforces that the
> code is removed when embedders no longer need it?

> >  
> > > In general, code that's only needed downstream should remain downstream.
> > 
> > This patch may be only used by Opera for now, but it can be useful for any
new
> > WebUI that
> > has mutiple instances. 
> > 
> > NTP doesn't use WebUIDataSource and implements URLDataSource directly, but
it
> > could be
> > a (though small) improvement to switch to WebUIDataSource as well.
> 
> If it can be useful for chromium, please update chromium so that a) chromium
> gets the benefit b) everyone can see that it's useful c) I can understand
> whether this patch is correct and necessary.

I realized it's not feasible to convert NTP to use WebUIDataSource as it
generates the main html docuement dynamically, also this is not testable in unit
test. So I added a comment to the header explaining why it's useful but not used
by chromium.

[arv (Not doing code reviews), 2013-08-12 15:18:55]

LGTM

Thanks for adding the comment.

[Evan Stade, 2013-08-12 16:38:28]

removing myself from reviewers.

[commit-bot: I haz the power, 2013-08-13 03:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shuais@opera.com/22150004/12001

[shuais_, 2013-08-13 04:00:17]

Thanks all.

[commit-bot: I haz the power, 2013-08-13 04:54:16]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[jam, 2013-08-13 21:36:45]

https://codereview.chromium.org/22150004/diff/12001/content/browser/webui/web...
File content/browser/webui/web_ui_data_source_impl.h (right):

https://codereview.chromium.org/22150004/diff/12001/content/browser/webui/web...
content/browser/webui/web_ui_data_source_impl.h:45: // Currently only used by
embedders for WebUIs with multiple instances, could
nit: method comments go on the interface and not the implementation

[shuais_, 2013-08-20 08:23:09]

On 2013/08/13 21:36:45, jam wrote:
>
https://codereview.chromium.org/22150004/diff/12001/content/browser/webui/web...
> File content/browser/webui/web_ui_data_source_impl.h (right):
> 
>
https://codereview.chromium.org/22150004/diff/12001/content/browser/webui/web...
> content/browser/webui/web_ui_data_source_impl.h:45: // Currently only used by
> embedders for WebUIs with multiple instances, could
> nit: method comments go on the interface and not the implementation

done

[commit-bot: I haz the power, 2013-08-21 08:26:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shuais@opera.com/22150004/46001

[commit-bot: I haz the power, 2013-08-21 12:25:37]

Change committed as 218710

[jam, 2013-08-25 07:22:38]

https://chromiumcodereview.appspot.com/22150004/diff/46001/content/public/bro...
File content/public/browser/web_ui_data_source.h (right):

https://chromiumcodereview.appspot.com/22150004/diff/46001/content/public/bro...
content/public/browser/web_ui_data_source.h:83: // have been useful for NTP as
well if it wasn't implementing URLDataSource
nit: please remove everything after the comment. per comment 11 NTP can't use
this, and either way it's a layering violation to mention a chrome concept in
content.

[shuais_, 2013-08-26 03:07:44]

On 2013/08/25 07:22:38, jam wrote:
>
https://chromiumcodereview.appspot.com/22150004/diff/46001/content/public/bro...
> File content/public/browser/web_ui_data_source.h (right):
> 
>
https://chromiumcodereview.appspot.com/22150004/diff/46001/content/public/bro...
> content/public/browser/web_ui_data_source.h:83: // have been useful for NTP as
> well if it wasn't implementing URLDataSource
> nit: please remove everything after the comment. per comment 11 NTP can't use
> this, and either way it's a layering violation to mention a chrome concept in
> content.

Fixed but can't commit as this issue is closed.. should I open a new issue for
the new fix or reopen this one?

[jam, 2013-08-26 14:18:58]

On 2013/08/26 03:07:44, shuais_ wrote:
> On 2013/08/25 07:22:38, jam wrote:
> >
>
https://chromiumcodereview.appspot.com/22150004/diff/46001/content/public/bro...
> > File content/public/browser/web_ui_data_source.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/22150004/diff/46001/content/public/bro...
> > content/public/browser/web_ui_data_source.h:83: // have been useful for NTP
as
> > well if it wasn't implementing URLDataSource
> > nit: please remove everything after the comment. per comment 11 NTP can't
use
> > this, and either way it's a layering violation to mention a chrome concept
in
> > content.
> 
> Fixed but can't commit as this issue is closed.. should I open a new issue for
> the new fix or reopen this one?

just upload a new issue