app.window: Replace linear view search with direct lookup.

app.window: Replace linear view search with direct lookup.

Added method RenderView::FromRoutingID to expose internal lookup function.

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

[Matt Giuca, 2012-11-26 05:05:30]

Hi Jeremy,

I found a method that does the routing-ID lookup just as you described in your
comment, so I think it's safe to use this.

I am concerned, however, that GetRoutingID (as implemented in
render_view_impl.cc) uses a static_cast to convert an IPC::Listener* to a
RenderViewImpl*. Is there any guarantee that looking up a routing ID will result
in a RenderView, as opposed to some other IPC::Listener? It would be good if
GetRoutingID were to explicitly check that the Listener is a RenderView, but I'm
not sure how to do that without RTTI. Perhaps you have a better understanding of
what a routing ID is.

Also: I was unable to find a good place to add a test case for the new
RenderView::FromRoutingID method. For example, render_view_test.h does not have
tests for the other static methods such as ForEach. Perhaps you can suggest a
way to test this, or maybe it is so simple that it does not need a test.

[jeremya, 2012-11-26 06:30:41]

+creis, who wrote the FromRoutingID function and has a much better general
understanding of this stuff than I do.

It looks from comments on the codereview for that patch
(https://chromiumcodereview.appspot.com/9108001) that it's okay though :)

[Matt Giuca, 2012-11-26 06:50:41]

On 2012/11/26 06:30:41, jeremya wrote:
> It looks from comments on the codereview for that patch
> (https://chromiumcodereview.appspot.com/9108001) that it's okay though :)

I don't see anything in the comments there reasoning about the downcast from
IPC::Listener* to RenderViewImpl*. Which part did you have in mind? My main
concern is that the limited uses inside render_view_impl.cc are guaranteed to
always be the correct type, but by exposing it as a public method of RenderView,
I will be allowing that guarantee to be violated (perhaps even by JavaScript
code).

[Charlie Reis, 2012-11-27 00:32:13]

On 2012/11/26 06:50:41, Matt Giuca wrote:
> On 2012/11/26 06:30:41, jeremya wrote:
> > It looks from comments on the codereview for that patch
> > (https://chromiumcodereview.appspot.com/9108001) that it's okay though :)
> 
> I don't see anything in the comments there reasoning about the downcast from
> IPC::Listener* to RenderViewImpl*. Which part did you have in mind? My main
> concern is that the limited uses inside render_view_impl.cc are guaranteed to
> always be the correct type, but by exposing it as a public method of
RenderView,
> I will be allowing that guarantee to be violated (perhaps even by JavaScript
> code).

Your concern sounds valid to me.  It's safe within RenderViewImpl because we
know we're only calling it with values that are from RenderViews.  Passing in
arbitrary routing IDs could lead to a bad cast, so I don't think we should
expose FromRoutingID as is.

I don't have great alternatives to suggest at the moment, beyond adding another
type of g_view_map to RenderViewImpl.  If you do go that route, it's worth
changing the existing FromRoutingID to use it to avoid future problems.

[benwells, 2012-11-27 01:35:59]

On 2012/11/27 00:32:13, creis wrote:
> On 2012/11/26 06:50:41, Matt Giuca wrote:
> > On 2012/11/26 06:30:41, jeremya wrote:
> > > It looks from comments on the codereview for that patch
> > > (https://chromiumcodereview.appspot.com/9108001) that it's okay though :)
> > 
> > I don't see anything in the comments there reasoning about the downcast from
> > IPC::Listener* to RenderViewImpl*. Which part did you have in mind? My main
> > concern is that the limited uses inside render_view_impl.cc are guaranteed
to
> > always be the correct type, but by exposing it as a public method of
> RenderView,
> > I will be allowing that guarantee to be violated (perhaps even by JavaScript
> > code).
> 
> Your concern sounds valid to me.  It's safe within RenderViewImpl because we
> know we're only calling it with values that are from RenderViews.  Passing in
> arbitrary routing IDs could lead to a bad cast, so I don't think we should
> expose FromRoutingID as is.
> 
> I don't have great alternatives to suggest at the moment, beyond adding
another
> type of g_view_map to RenderViewImpl.  If you do go that route, it's worth
> changing the existing FromRoutingID to use it to avoid future problems.

I don't think the linear search is a problem. The problem is that it isn't
abstracted anywhere. I'd be happy just adding RVHFromID (or whatever it
specifically is that we need) to ShellWindowRegistry ... or something like that.

[Matt Giuca, 2012-11-27 01:51:08]

On 2012/11/27 00:32:13, creis wrote:
> I don't have great alternatives to suggest at the moment, beyond adding
another
> type of g_view_map to RenderViewImpl.  If you do go that route, it's worth
> changing the existing FromRoutingID to use it to avoid future problems.

Sounds good to me. In fact, I already (mostly) implemented this yesterday, but
then I discovered the existing FromRoutingID and figured that I should use it.
I'm happy to revert to my code from yesterday, with a dedicated map from routing
ID to RenderView*.

On 2012/11/27 01:35:59, benwells wrote:
> I don't think the linear search is a problem. The problem is that it isn't
> abstracted anywhere. I'd be happy just adding RVHFromID (or whatever it
> specifically is that we need) to ShellWindowRegistry ... or something like
that.

Yes, I definitely want to abstract this away. I think the interface proposed in
this CL (adding FromRoutingID static method to the RenderView) is a good
abstraction. We can easily keep the same algorithm but move the code to there.

But having done that, I don't see why we shouldn't also optimize away the linear
search (which probably makes sense to do in another CL).

[Matt Giuca, 2012-11-28 01:55:01]

Here is a CL to fix the issue with FromRoutingID:
https://codereview.chromium.org/11316209

I'll wait until that is taken care of, then push forward with this one (at which
point it should be acceptable to expose FromRoutingID as a public API).

[Matt Giuca, 2012-12-03 07:33:37]

That CL is now committed (revision 170708), so please take another look at this
(jeremya).

Remember that my initial concern was that GetRoutingID might return an invalid
pointer due to looking up something other than a RenderView and downcasting it.
That is no longer a concern since there is an explicit routing ID -> RenderView
map which it uses to do the lookup. No changes were required to this patch.

Also, can you please start a try for me again. Thanks :)

[Charlie Reis, 2012-12-03 19:29:39]

https://codereview.chromium.org/11316160/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11316160/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:773: RenderView*
RenderView::FromRoutingID(int routing_id) {
Why do we need two functions?  Just move the other code here.  Also, you
shouldn't need content:: in this file.

[Matt Giuca, 2012-12-03 23:22:22]

https://codereview.chromium.org/11316160/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11316160/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:773: RenderView*
RenderView::FromRoutingID(int routing_id) {
On 2012/12/03 19:29:39, creis wrote:
> Why do we need two functions?  Just move the other code here.

content::FromRoutingID returns a RenderViewImpl*; RenderView::FromRoutingID
returns a RenderView*. There is other code in this module which uses
content::FromRoutingID and accesses private methods of RenderViewImpl.

Actually, they just access the webview() method, which is exposed as
RenderView::GetWebView(), so I could change all calls to use the public
GetWebView method. This seems like unnecessary indirection to me. (Since we are
in the RenderViewImpl class, why use public methods and take an extra virtual
method lookup?) But I can change it if you think it's worth avoiding the two
methods.

> Also, you shouldn't need content:: in this file.

You do if you want to refer to the private implementation of FromRoutingID.
Without content::, it implicitly refers to the public RenderView::FromRoutingID
method. I'll get rid of the content:: qualifiers if I am to go ahead with
removing the private version of FromRoutingID.

[Charlie Reis, 2012-12-03 23:45:58]

https://codereview.chromium.org/11316160/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11316160/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:773: RenderView*
RenderView::FromRoutingID(int routing_id) {
On 2012/12/03 23:22:22, Matt Giuca wrote:
> On 2012/12/03 19:29:39, creis wrote:
> > Why do we need two functions?  Just move the other code here.
> 
> content::FromRoutingID returns a RenderViewImpl*; RenderView::FromRoutingID
> returns a RenderView*. There is other code in this module which uses
> content::FromRoutingID and accesses private methods of RenderViewImpl.
> 
> Actually, they just access the webview() method, which is exposed as
> RenderView::GetWebView(), so I could change all calls to use the public
> GetWebView method. This seems like unnecessary indirection to me. (Since we
are
> in the RenderViewImpl class, why use public methods and take an extra virtual
> method lookup?) But I can change it if you think it's worth avoiding the two
> methods.
> 
> > Also, you shouldn't need content:: in this file.
> 
> You do if you want to refer to the private implementation of FromRoutingID.
> Without content::, it implicitly refers to the public
RenderView::FromRoutingID
> method. I'll get rid of the content:: qualifiers if I am to go ahead with
> removing the private version of FromRoutingID.

Perhaps we should mimic FromWebView to stay consistent within this file.  Looks
like it has the same use case requirements.

[Matt Giuca, 2012-12-03 23:50:47]

On 2012/12/03 23:45:58, creis wrote:

> Perhaps we should mimic FromWebView to stay consistent within this file. 
Looks
> like it has the same use case requirements.

Right. So I'll keep the two implementations of FromRoutingID, but I'll make the
private one a static method of RenderViewImpl instead of a global function,
which will avoid the need for content::.

[Matt Giuca, 2012-12-04 07:52:56]

Made those changes. Please take another look, and re-run the trybots.

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

Thanks!  LGTM.

[commit-bot: I haz the power, 2012-12-04 23:20:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/11316160/9001

[commit-bot: I haz the power, 2012-12-04 23:20:47]

Presubmit check for 11316160-9001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/renderer/extensions

Presubmit checks took 1.3s to calculate.

[Matt Giuca, 2012-12-04 23:24:26]

Hey jeremya, can I get your LGTM (since you are an OWNER). Thanks.

[jeremya, 2012-12-04 23:57:34]

chrome/browser/extensions LGTM :)

[jeremya, 2012-12-04 23:57:48]

chrome/renderer/extensions even :p

[Matt Giuca, 2012-12-04 23:58:56]

On 2012/12/04 23:57:48, jeremya wrote:
> chrome/renderer/extensions even :p

Cheers!

[commit-bot: I haz the power, 2012-12-04 23:59:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/11316160/9001

[commit-bot: I haz the power, 2012-12-05 01:36:08]

Change committed as 171117