RenderFrame: flesh out Observer

RenderFrame: flesh out Observer

Add four new events to RenderFrameObserver: DidCreateDataSource,
DidStartProvisionalLoad, DidFailProvisionalLoad, DidFinishLoad; add code to send
the former three as needed.

TEST=unit,trybot
BUG=none

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

[nasko, 2014-01-13 22:55:32]

LGTM with a nit.

https://codereview.chromium.org/137463002/diff/20001/content/public/renderer/...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/137463002/diff/20001/content/public/renderer/...
content/public/renderer/render_frame_observer.h:47: blink::WebDataSource*
datasource) { }
nit: data_source or ds

[jam, 2014-01-13 22:56:15]

On 2014/01/13 22:54:01, Elly Jones wrote:

1) why are there 3 reviewers?
2) in general for content API changes, we add methods as they are used and not
in-advance. that way we ensure that methods need only the parameters that are
required, and we don't add methods that may not be needed.

[Elly Fong-Jones, 2014-01-14 14:44:44]

On 2014/01/13 22:56:15, jam wrote:
> On 2014/01/13 22:54:01, Elly Jones wrote:
> 
> 1) why are there 3 reviewers?

I added nasko because he suggested this change, then jochen as an owner, and
then when I realized he was already asleep I added you and forgot to remove him.

> 2) in general for content API changes, we add methods as they are used and not
> in-advance. that way we ensure that methods need only the parameters that are
> required, and we don't add methods that may not be needed.

Indeed. This change is meant to go with a companion CL (not up yet) that makes
NetErrorHelper a RenderFrameObserver rather than a RenderViewObserver. I can put
that CL up before asking for review here if you prefer.

[Elly Fong-Jones, 2014-01-14 19:57:10]

On 2014/01/14 14:44:44, Elly Jones wrote:
> On 2014/01/13 22:56:15, jam wrote:
> > On 2014/01/13 22:54:01, Elly Jones wrote:
> > 
> > 1) why are there 3 reviewers?
> 
> I added nasko because he suggested this change, then jochen as an owner, and
> then when I realized he was already asleep I added you and forgot to remove
him.
> 
> > 2) in general for content API changes, we add methods as they are used and
not
> > in-advance. that way we ensure that methods need only the parameters that
are
> > required, and we don't add methods that may not be needed.
> 
> Indeed. This change is meant to go with a companion CL (not up yet) that makes
> NetErrorHelper a RenderFrameObserver rather than a RenderViewObserver. I can
put
> that CL up before asking for review here if you prefer.

Companion CL: https://codereview.chromium.org/138583002/

[Elly Fong-Jones, 2014-01-15 16:19:19]

On 2014/01/14 19:57:10, Elly Jones wrote:
> On 2014/01/14 14:44:44, Elly Jones wrote:
> > On 2014/01/13 22:56:15, jam wrote:
> > > On 2014/01/13 22:54:01, Elly Jones wrote:
> > > 
> > > 1) why are there 3 reviewers?
> > 
> > I added nasko because he suggested this change, then jochen as an owner, and
> > then when I realized he was already asleep I added you and forgot to remove
> him.
> > 
> > > 2) in general for content API changes, we add methods as they are used and
> not
> > > in-advance. that way we ensure that methods need only the parameters that
> are
> > > required, and we don't add methods that may not be needed.
> > 
> > Indeed. This change is meant to go with a companion CL (not up yet) that
makes
> > NetErrorHelper a RenderFrameObserver rather than a RenderViewObserver. I can
> put
> > that CL up before asking for review here if you prefer.
> 
> Companion CL: https://codereview.chromium.org/138583002/

jam: does this CL look ok now?

[Elly Fong-Jones, 2014-01-16 16:13:41]

On 2014/01/15 16:19:19, Elly Jones wrote:
> On 2014/01/14 19:57:10, Elly Jones wrote:
> > On 2014/01/14 14:44:44, Elly Jones wrote:
> > > On 2014/01/13 22:56:15, jam wrote:
> > > > On 2014/01/13 22:54:01, Elly Jones wrote:
> > > > 
> > > > 1) why are there 3 reviewers?
> > > 
> > > I added nasko because he suggested this change, then jochen as an owner,
and
> > > then when I realized he was already asleep I added you and forgot to
remove
> > him.
> > > 
> > > > 2) in general for content API changes, we add methods as they are used
and
> > not
> > > > in-advance. that way we ensure that methods need only the parameters
that
> > are
> > > > required, and we don't add methods that may not be needed.
> > > 
> > > Indeed. This change is meant to go with a companion CL (not up yet) that
> makes
> > > NetErrorHelper a RenderFrameObserver rather than a RenderViewObserver. I
can
> > put
> > > that CL up before asking for review here if you prefer.
> > 
> > Companion CL: https://codereview.chromium.org/138583002/
> 
> jam: does this CL look ok now?

ping?

[jam, 2014-01-16 16:18:28]

sorry I just saw this. for some reason your replies by email didn't show up
on rietveld.

i'm not sure why there is a different companion cl. Can you combine them
into one cl? as i mentioned in the other one, that way when we add a new
method to RFObserver it's clear where it's used.


On Thu, Jan 16, 2014 at 8:13 AM, <ellyjones@chromium.org> wrote:

> On 2014/01/15 16:19:19, Elly Jones wrote:
>
>> On 2014/01/14 19:57:10, Elly Jones wrote:
>> > On 2014/01/14 14:44:44, Elly Jones wrote:
>> > > On 2014/01/13 22:56:15, jam wrote:
>> > > > On 2014/01/13 22:54:01, Elly Jones wrote:
>> > > >
>> > > > 1) why are there 3 reviewers?
>> > >
>> > > I added nasko because he suggested this change, then jochen as an
>> owner,
>>
> and
>
>> > > then when I realized he was already asleep I added you and forgot to
>>
> remove
>
>> > him.
>> > >
>> > > > 2) in general for content API changes, we add methods as they are
>> used
>>
> and
>
>> > not
>> > > > in-advance. that way we ensure that methods need only the parameters
>>
> that
>
>> > are
>> > > > required, and we don't add methods that may not be needed.
>> > >
>> > > Indeed. This change is meant to go with a companion CL (not up yet)
>> that
>> makes
>> > > NetErrorHelper a RenderFrameObserver rather than a
>> RenderViewObserver. I
>>
> can
>
>> > put
>> > > that CL up before asking for review here if you prefer.
>> >
>> > Companion CL: https://codereview.chromium.org/138583002/
>>
>
>  jam: does this CL look ok now?
>>
>
> ping?
>
> https://codereview.chromium.org/137463002/
>

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

[Elly Fong-Jones, 2014-01-16 20:45:22]

I have smashed this CL together with its companion CL. PTAL?

On 2014/01/16 16:18:28, jam wrote:
> sorry I just saw this. for some reason your replies by email didn't show up
> on rietveld.
> 
> i'm not sure why there is a different companion cl. Can you combine them
> into one cl? as i mentioned in the other one, that way when we add a new
> method to RFObserver it's clear where it's used.
> 
> 
> On Thu, Jan 16, 2014 at 8:13 AM, <mailto:ellyjones@chromium.org> wrote:
> 
> > On 2014/01/15 16:19:19, Elly Jones wrote:
> >
> >> On 2014/01/14 19:57:10, Elly Jones wrote:
> >> > On 2014/01/14 14:44:44, Elly Jones wrote:
> >> > > On 2014/01/13 22:56:15, jam wrote:
> >> > > > On 2014/01/13 22:54:01, Elly Jones wrote:
> >> > > >
> >> > > > 1) why are there 3 reviewers?
> >> > >
> >> > > I added nasko because he suggested this change, then jochen as an
> >> owner,
> >>
> > and
> >
> >> > > then when I realized he was already asleep I added you and forgot to
> >>
> > remove
> >
> >> > him.
> >> > >
> >> > > > 2) in general for content API changes, we add methods as they are
> >> used
> >>
> > and
> >
> >> > not
> >> > > > in-advance. that way we ensure that methods need only the parameters
> >>
> > that
> >
> >> > are
> >> > > > required, and we don't add methods that may not be needed.
> >> > >
> >> > > Indeed. This change is meant to go with a companion CL (not up yet)
> >> that
> >> makes
> >> > > NetErrorHelper a RenderFrameObserver rather than a
> >> RenderViewObserver. I
> >>
> > can
> >
> >> > put
> >> > > that CL up before asking for review here if you prefer.
> >> >
> >> > Companion CL: https://codereview.chromium.org/138583002/
> >>
> >
> >  jam: does this CL look ok now?
> >>
> >
> > ping?
> >
> > https://codereview.chromium.org/137463002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-01-16 22:17:08]

can you also apply the comment from the other cl? thanks

https://codereview.chromium.org/137463002/diff/190001/content/public/renderer...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/137463002/diff/190001/content/public/renderer...
content/public/renderer/render_frame_observer.h:39: virtual void
DidCreateDataSource(blink::WebFrame* frame,
i don't see this being used in the new helper? let's leave it out until we have
a use case for it

https://codereview.chromium.org/137463002/diff/190001/content/public/renderer...
content/public/renderer/render_frame_observer.h:43: virtual void
DidStartProvisionalLoad(blink::WebFrame* frame) { }
in the new methods, can you remove the blink::WebFrame? that is now redundant
since there's one WebFrame per RenderFrame

https://codereview.chromium.org/137463002/diff/190001/content/public/renderer...
content/public/renderer/render_frame_observer.h:44: virtual void
DidFailProvisionalLoad(blink::WebFrame* frame,
ditto

[Elly Fong-Jones, 2014-01-21 20:23:11]

On 2014/01/16 22:17:08, jam wrote:
> can you also apply the comment from the other cl? thanks
> 
>
https://codereview.chromium.org/137463002/diff/190001/content/public/renderer...
> File content/public/renderer/render_frame_observer.h (right):
> 
>
https://codereview.chromium.org/137463002/diff/190001/content/public/renderer...
> content/public/renderer/render_frame_observer.h:39: virtual void
> DidCreateDataSource(blink::WebFrame* frame,
> i don't see this being used in the new helper? let's leave it out until we
have
> a use case for it
> 
>
https://codereview.chromium.org/137463002/diff/190001/content/public/renderer...
> content/public/renderer/render_frame_observer.h:43: virtual void
> DidStartProvisionalLoad(blink::WebFrame* frame) { }
> in the new methods, can you remove the blink::WebFrame? that is now redundant
> since there's one WebFrame per RenderFrame
> 
>
https://codereview.chromium.org/137463002/diff/190001/content/public/renderer...
> content/public/renderer/render_frame_observer.h:44: virtual void
> DidFailProvisionalLoad(blink::WebFrame* frame,
> ditto

Done.

[jam, 2014-01-22 03:15:34]

thanks, lgtm with comments

https://codereview.chromium.org/137463002/diff/250001/chrome/renderer/net/net...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/137463002/diff/250001/chrome/renderer/net/net...
chrome/renderer/net/net_error_helper.cc:143: frame->loadHTMLString(html,
GURL(kUnreachableWebDataURL), failed_url, true);
this can just be render_frame()->GetWebFrame()->loadHTMLString right?

https://codereview.chromium.org/137463002/diff/250001/chrome/renderer/net/net...
chrome/renderer/net/net_error_helper.cc:179: blink::WebFrame* frame =
web_view->mainFrame();
ditto

https://codereview.chromium.org/137463002/diff/250001/content/public/renderer...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/137463002/diff/250001/content/public/renderer...
content/public/renderer/render_frame_observer.h:39: virtual void
DidStartProvisionalLoad() { }
nit: no space inside brackets, also below

[commit-bot: I haz the power, 2014-01-22 23:44:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/137463002/380001

[commit-bot: I haz the power, 2014-01-22 23:44:22]

Failed to apply patch for chrome/renderer/content_settings_observer.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/content_settings_observer.cc
  Hunk #1 FAILED at 209.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/renderer/content_settings_observer.cc.rej

Patch:       chrome/renderer/content_settings_observer.cc
Index: chrome/renderer/content_settings_observer.cc
diff --git a/chrome/renderer/content_settings_observer.cc
b/chrome/renderer/content_settings_observer.cc
index
98a28980595bb54a72f5414b4038642149cf079c..1414e48491ebd2ca7e6962170569cdbd897b7a7a
100644
--- a/chrome/renderer/content_settings_observer.cc
+++ b/chrome/renderer/content_settings_observer.cc
@@ -209,7 +209,8 @@ bool ContentSettingsObserver::OnMessageReceived(const
IPC::Message& message) {
 }
 
 void ContentSettingsObserver::DidCommitProvisionalLoad(
-    WebFrame* frame, bool is_new_navigation) {
+    bool is_new_navigation) {
+  blink::WebFrame* frame = render_frame()->GetWebFrame();
   if (frame->parent())
     return;  // Not a top-level navigation.

[commit-bot: I haz the power, 2014-01-23 16:15:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/137463002/500001

[commit-bot: I haz the power, 2014-01-23 18:16:13]

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

[nasko, 2014-01-28 20:41:37]

A round of comments.

https://codereview.chromium.org/137463002/diff/780001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/137463002/diff/780001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:380: if
(!render_frame->GetWebFrame()->parent()) {
We just found that the parenting relationship isn't properly established at the
time of this call, so we have to revert this back to asking for the main frame.

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
content/public/renderer/render_frame.h:81: virtual void OnStop() = 0;
This doesn't need to be public, it is just an IPC endpoint for receiving
messages.

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
content/public/renderer/render_frame_observer.h:37: // Called when a load is
stopped.
nit: This is explicitly stopped by the user/browser, so it might be useful to
clarify it in the comment.

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
content/public/renderer/render_frame_observer.h:45: virtual void DidAbortLoad()
{}
Let's get rid of DidAbortLoad, as it was never in RVO and you are moving to
OnStop.

https://codereview.chromium.org/137463002/diff/780001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/137463002/diff/780001/content/renderer/render...
content/renderer/render_frame_impl.cc:1019:
FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
In all other cases we notify RVO before RFO, let's keep them consistent here as
well.

https://codereview.chromium.org/137463002/diff/780001/content/renderer/render...
content/renderer/render_frame_impl.cc:1281: #if defined(ENABLE_PLUGINS)
Why is this define moved?

https://codereview.chromium.org/137463002/diff/780001/content/renderer/render...
content/renderer/render_frame_impl.cc:1285:
FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidAbortLoad());
Let's remove this.

[Elly Fong-Jones, 2014-01-28 21:34:19]

https://codereview.chromium.org/137463002/diff/780001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/137463002/diff/780001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:380: if
(!render_frame->GetWebFrame()->parent()) {
On 2014/01/28 20:41:39, nasko wrote:
> We just found that the parenting relationship isn't properly established at
the
> time of this call, so we have to revert this back to asking for the main
frame.

Done.

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
content/public/renderer/render_frame.h:81: virtual void OnStop() = 0;
On 2014/01/28 20:41:39, nasko wrote:
> This doesn't need to be public, it is just an IPC endpoint for receiving
> messages.

Yes it does - it is not a "real" IPC endpoint but is instead called by
RenderViewObserver.

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
content/public/renderer/render_frame_observer.h:37: // Called when a load is
stopped.
On 2014/01/28 20:41:39, nasko wrote:
> nit: This is explicitly stopped by the user/browser, so it might be useful to
> clarify it in the comment.

Done.

https://codereview.chromium.org/137463002/diff/780001/content/public/renderer...
content/public/renderer/render_frame_observer.h:45: virtual void DidAbortLoad()
{}
On 2014/01/28 20:41:39, nasko wrote:
> Let's get rid of DidAbortLoad, as it was never in RVO and you are moving to
> OnStop.

Done.

https://codereview.chromium.org/137463002/diff/780001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/137463002/diff/780001/content/renderer/render...
content/renderer/render_frame_impl.cc:1019:
FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
On 2014/01/28 20:41:39, nasko wrote:
> In all other cases we notify RVO before RFO, let's keep them consistent here
as
> well.

Done.

https://codereview.chromium.org/137463002/diff/780001/content/renderer/render...
content/renderer/render_frame_impl.cc:1281: #if defined(ENABLE_PLUGINS)
On 2014/01/28 20:41:39, nasko wrote:
> Why is this define moved?

Done.

https://codereview.chromium.org/137463002/diff/780001/content/renderer/render...
content/renderer/render_frame_impl.cc:1285:
FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidAbortLoad());
On 2014/01/28 20:41:39, nasko wrote:
> Let's remove this.

Done.

[nasko, 2014-01-28 21:34:36]

LGTM on change to OnStop.

[commit-bot: I haz the power, 2014-01-28 22:18:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/137463002/800001

[commit-bot: I haz the power, 2014-01-29 11:39:43]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-29 16:17:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/137463002/800001

[commit-bot: I haz the power, 2014-01-29 17:45:08]

Change committed as 247717