chrome.tabs.onUpdated doesn't called with 'loading' twice.

chrome.tabs.onUpdated doesn't called with 'loading' twice.

- Adding several tests for in-page navigations.
- tabs.onUpdate() doesn't have 'loading' status if previous tabs.onUpdate() have 'loading'.

BUG=465709
TEST=browser_tests --gtest_filter=ExtensionApiTest.TabsOnUpdated

[limasdf, 2015-05-25 13:41:28]

limasdf@gmail.com changed reviewers:
+ kalman@chromium.org

[limasdf, 2015-05-25 13:41:36]

Patchset #1 (id:1) has been deleted

[limasdf, 2015-05-25 13:41:43]

Patchset #1 (id:20001) has been deleted

[limasdf, 2015-05-25 13:42:27]

PTAL when you have time.

[not at google - send to devlin, 2015-05-27 15:59:15]

Two more things:
1. Add a test.
2. I'm actually thinking that the bug here is that we send a "loading" when we
should be sending an "updated". Arguably any sort of navigation - including
in-page - should be sending onUpdated.

https://codereview.chromium.org/1156043002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/1156043002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:144: this,
content::NOTIFICATION_NAV_ENTRY_COMMITTED,
Could you add:

TODO(limasdf): Attach WebContentsObservers to |contents| rather than registering
for notifications.

(no need to actually do it anytime soon! it's not high impact, it's just good to
keep track of these things)

https://codereview.chromium.org/1156043002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:496: if (type ==
content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
can you update this to be a switch?

[not at google - send to devlin, 2015-05-28 15:08:18]

On 2015/05/27 15:59:15, kalman wrote:
> Two more things:
> 1. Add a test.
> 2. I'm actually thinking that the bug here is that we send a "loading" when we
> should be sending an "updated". Arguably any sort of navigation - including
> in-page - should be sending onUpdated.

Correction: I should have read the API docs, I actually think we should be
firing the onUpdated event without the "status" set at all:
https://developer.chrome.com/extensions/tabs#property-onUpdated-changeInfo
(just the URL)

[limasdf, 2015-05-28 16:03:34]

On 2015/05/28 15:08:18, kalman wrote:
> On 2015/05/27 15:59:15, kalman wrote:
> > Two more things:
> > 1. Add a test.
> > 2. I'm actually thinking that the bug here is that we send a "loading" when
we
> > should be sending an "updated". Arguably any sort of navigation - including
> > in-page - should be sending onUpdated.
> 
> Correction: I should have read the API docs, I actually think we should be
> firing the onUpdated event without the "status" set at all:
> https://developer.chrome.com/extensions/tabs#property-onUpdated-changeInfo
> (just the URL)

Thank you for the review, kalman!

I'm a little confused. Firing onUpdated event, that means some value is updated.
But if we don't set any value including the 'status', How a user can recognize
the value updated?
Is it enough just unset the 'status'?

[not at google - send to devlin, 2015-05-28 16:06:55]

On 2015/05/28 16:03:34, limasdf wrote:
> On 2015/05/28 15:08:18, kalman wrote:
> > On 2015/05/27 15:59:15, kalman wrote:
> > > Two more things:
> > > 1. Add a test.
> > > 2. I'm actually thinking that the bug here is that we send a "loading"
when
> we
> > > should be sending an "updated". Arguably any sort of navigation -
including
> > > in-page - should be sending onUpdated.
> > 
> > Correction: I should have read the API docs, I actually think we should be
> > firing the onUpdated event without the "status" set at all:
> > https://developer.chrome.com/extensions/tabs#property-onUpdated-changeInfo
> > (just the URL)
> 
> Thank you for the review, kalman!
> 
> I'm a little confused. Firing onUpdated event, that means some value is
updated.
> But if we don't set any value including the 'status', How a user can recognize
> the value updated?
> Is it enough just unset the 'status'?

It looks to me like onUpdated sends a dictionary of what changed, and the status
is optional. Given here the status didn't actually change, just the URL, I think
that's all you need to set.

[limasdf, 2015-06-16 16:12:56]

Patchset #2 (id:60001) has been deleted

[limasdf, 2015-06-18 16:39:08]

Patchset #2 (id:80001) has been deleted

[limasdf, 2015-06-28 23:11:00]

Patchset #2 (id:100001) has been deleted

[limasdf, 2015-06-30 15:51:40]

Patchset #2 (id:120001) has been deleted

[limasdf, 2015-07-01 11:32:12]

Patchset #3 (id:160001) has been deleted

[limasdf, 2015-07-01 11:32:20]

Patchset #2 (id:140001) has been deleted

[limasdf, 2015-07-01 12:11:28]

Patchset #2 (id:180001) has been deleted

[limasdf, 2015-07-01 12:11:40]

Patchset #2 (id:200001) has been deleted

[limasdf, 2015-07-01 12:24:46]

Patchset #2 (id:220001) has been deleted

[limasdf, 2015-07-01 12:37:54]

> 1. Add a test.
Done.

> Correction: I should have read the API docs, I actually think we should be
> firing the onUpdated event without the "status" set at all:
Done.

PTAL when you have time.

This patch covers history API(both pushState and replaceState).
Actually this CL affects only the result of 'history.replaceState'. it looks
like current code for 'history.pushState' looks fine.
(You can see tests that I added)

https://codereview.chromium.org/1156043002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/1156043002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:144: this,
content::NOTIFICATION_NAV_ENTRY_COMMITTED,
On 2015/05/27 15:59:15, kalman wrote:
> Could you add:
> 
> TODO(limasdf): Attach WebContentsObservers to |contents| rather than
registering
> for notifications.
> 
> (no need to actually do it anytime soon! it's not high impact, it's just good
to
> keep track of these things)

Done.

https://codereview.chromium.org/1156043002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:496: if (type ==
content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
On 2015/05/27 15:59:15, kalman wrote:
> can you update this to be a switch?

Done.

[limasdf, 2015-07-01 15:22:23]

On 2015/07/01 12:37:54, limasdf wrote:
> > 1. Add a test.
> Done.
> 
> > Correction: I should have read the API docs, I actually think we should be
> > firing the onUpdated event without the "status" set at all:
> Done.
> 
> PTAL when you have time.
> 
> This patch covers history API(both pushState and replaceState).
> Actually this CL affects only the result of 'history.replaceState'. it looks
> like current code for 'history.pushState' looks fine.
> (You can see tests that I added)
> 
>
https://codereview.chromium.org/1156043002/diff/40001/chrome/browser/extensio...
> File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):
> 
>
https://codereview.chromium.org/1156043002/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/api/tabs/tabs_event_router.cc:144: this,
> content::NOTIFICATION_NAV_ENTRY_COMMITTED,
> On 2015/05/27 15:59:15, kalman wrote:
> > Could you add:
> > 
> > TODO(limasdf): Attach WebContentsObservers to |contents| rather than
> registering
> > for notifications.
> > 
> > (no need to actually do it anytime soon! it's not high impact, it's just
good
> to
> > keep track of these things)
> 
> Done.
> 
>
https://codereview.chromium.org/1156043002/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/api/tabs/tabs_event_router.cc:496: if (type ==
> content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
> On 2015/05/27 15:59:15, kalman wrote:
> > can you update this to be a switch?
> 
> Done.

Sorry to be late, btw.
please take a look when you have time(no rush).

[not at google - send to devlin, 2015-07-01 15:57:15]

Lots of comments here, sorry.

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:107:
base::DictionaryValue* changed_properties = new base::DictionaryValue();
To imply type-safe pointer ownership transfer to the caller, make this a
scoped_ptr<base::DictionaryValue> then return changed_properties.Pass() from
this function.

And sorry for the small amount of churn, but could you change UpdateLoadState
and DidNavigate to do the same?

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:158: //               
registering for notifications.
Typically we don't indent comments like this.

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:422:
changed_properties.reset(entry->DidNavigate(contents));
and then, following on from my comment above, these would simply be:
  changed_properties = entry->DidNavigate(contents);

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:428:
changed_properties.reset(entry->UpdateUrl(contents));
It's actually not clear to me - or it wouldn't be outside the context of this CL
- why a NONE status implies the URL was updated. In other words, why the
*current* tab status (which in the case of NONE is very general) implies
something about the *change to the URL* (which is very specific).

For example - take the bug about the title changing -
https://code.google.com/p/chromium/issues/detail?id=458839. How would that fit
in here? You'd need to pass the |status| in as NONE but then how do you know if
it's a URL change, versus a title change?

I think the problem is that we're trying to distinguish between 2 different
kinds of updates, where really we shouldn't. Instead of those 3 methods
DidNavigate/UpdateLoadState/UpdateUrl we should just have an "Update" method and
a "Reset" method. "Reset" would reset all the state to its default and set
change values for anything that changed. "Update" would detect what has changed
by holding onto the previous values, in this case, the load state
(api::tabs::TabStatus) and a URL:

scoped_ptr<DictionaryValue> changed(new DictionaryValue);
if (status_ == 'loading' && !contents->IsLoading()) {
  // load status has changed from 'loading' to 'complete'.
  changed->SetString('status', status_);
  entry->tab_status = 'complete';
}
GURL new_url = contents->GetCommittedURL();
if (url_ != new_url) {
  // URL has changed. This may be because the loading state changed,
  // or it may be an in-page navigation like a #-navigation or
  // history.push/replaceState.
  changed->SetString('url', new_url);
  url_ = new_url;
}
// Then later, you or I or somebody else can add:
std::string new_title = ...;
if (title_ -> new_title) {
  changed->SetString('title', new_title);
  title_ = new_title;
}
return changed.Pass();

Does that make sense? Would it work? Sorry for this big change!

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:528: if (commit->type ==
content::NavigationType::NAVIGATION_TYPE_IN_PAGE) {
It may be simpler to just do:

if (commit->is_in_page) {
  ...
} else {
  ...
}

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:529: //
'History.ReplaceState()' falls here. Just update url only.
It's not just that, it's also hash fragment navigation? I'd make the comment
more like "In-page navigation includes #-navigation and
history.push/replaceState.

(and yep I see that you said that pushState already "works" - which is
surprising to me)

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/tabs/on_updated/historyPushState/a.html
(right):

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/tabs/on_updated/historyPushState/a.html:1:
<!--
This new test isn't a copy, even though git thinks it is, and it will be a
confusing revision history if it's committed that way.
It's easy to fix: when you upload the CL, use the "--no-find-copies" flag.

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/tabs/on_updated/historyReplaceStateUpdateUrl/a.js
(right):

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/tabs/on_updated/historyReplaceStateUpdateUrl/a.js:8:
history.replaceState(null, null, "#a");
replaceState is more interesting if you don't specify a hash fragment at all.

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/tabs/on_updated/test.js (right):

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/tabs/on_updated/test.js:170: }
Could you also test simple hash fragments, like location.replace/assign('#foo')?

[limasdf, 2015-07-02 12:36:36]

Patchset #3 (id:170012) has been deleted

[limasdf, 2015-07-02 12:36:47]

Patchset #3 (id:270001) has been deleted

[limasdf, 2015-07-02 13:25:42]

Patchset #3 (id:290001) has been deleted

[limasdf, 2015-07-02 15:24:28]

Patchset #3 (id:310001) has been deleted

[limasdf, 2015-07-02 15:24:34]

Patchset #3 (id:330001) has been deleted

[limasdf, 2015-07-05 15:07:45]

Patchset #3 (id:350001) has been deleted

[limasdf, 2015-07-05 16:05:52]

Patchset #3 (id:370001) has been deleted

[limasdf, 2015-07-05 16:10:50]

Patchset #3 (id:390001) has been deleted

[limasdf, 2015-07-05 16:28:38]

Patchset #3 (id:410001) has been deleted

[limasdf, 2015-07-05 17:23:38]

Patchset #3 (id:430001) has been deleted

[limasdf, 2015-07-05 17:23:46]

Patchset #3 (id:450001) has been deleted

[limasdf, 2015-07-05 18:01:11]

Patchset #3 (id:470001) has been deleted

[limasdf, 2015-07-06 15:25:23]

Patchset #3 (id:480001) has been deleted

[limasdf, 2015-07-06 15:25:39]

Patchset #3 (id:480001) has been deleted

[limasdf, 2015-07-06 15:25:47]

Patchset #3 (id:500001) has been deleted

[limasdf, 2015-07-06 15:50:36]

Patchset #3 (id:520001) has been deleted

[limasdf, 2015-07-06 15:54:29]

Patchset #3 (id:540001) has been deleted

[limasdf, 2015-07-06 15:55:40]

Patchset #3 (id:560001) has been deleted

[limasdf, 2015-07-06 16:35:24]

Patchset #3 (id:580001) has been deleted

[limasdf, 2015-07-06 17:14:44]

when you have time -

Sorry I think this CL doesn't looks good.
But it would mean the world to us if you leave some comments with wisdom.

1. Tests look good?
 - in the tests, in-page navigations(including history APIs) generate 'loading'
twice, 'complete' twice. On crbug.com/465709, the bug reporter said it's wrong.

2. Interestingly, on Youtube.com, 
content::NOTIFICATION_NAV_ENTRY_COMMITTED comes again
with |webcontents->IsLoading() == true| before the completion of page loading.
this is weird. Only Youtube.com acts like that. So this CL just ignore the
second contents::NOTIFICATION_NAV_ENTRY_COMMITED.

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:107:
base::DictionaryValue* changed_properties = new base::DictionaryValue();
On 2015/07/01 15:57:15, kalman wrote:
> To imply type-safe pointer ownership transfer to the caller, make this a
> scoped_ptr<base::DictionaryValue> then return changed_properties.Pass() from
> this function.
> 
> And sorry for the small amount of churn, but could you change UpdateLoadState
> and DidNavigate to do the same?

Done. Yeah they should be.

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:158: //               
registering for notifications.
On 2015/07/01 15:57:15, kalman wrote:
> Typically we don't indent comments like this.

Done.

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:422:
changed_properties.reset(entry->DidNavigate(contents));
On 2015/07/01 15:57:15, kalman wrote:
> and then, following on from my comment above, these would simply be:
>   changed_properties = entry->DidNavigate(contents);

Done.

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:428:
changed_properties.reset(entry->UpdateUrl(contents));
On 2015/07/01 15:57:15, kalman wrote:
> It's actually not clear to me - or it wouldn't be outside the context of this
CL
> - why a NONE status implies the URL was updated. In other words, why the
> *current* tab status (which in the case of NONE is very general) implies
> something about the *change to the URL* (which is very specific).
> 
> For example - take the bug about the title changing -
> https://code.google.com/p/chromium/issues/detail?id=458839. How would that fit
> in here? You'd need to pass the |status| in as NONE but then how do you know
if
> it's a URL change, versus a title change?
> 
> I think the problem is that we're trying to distinguish between 2 different
> kinds of updates, where really we shouldn't. Instead of those 3 methods
> DidNavigate/UpdateLoadState/UpdateUrl we should just have an "Update" method
and
> a "Reset" method. "Reset" would reset all the state to its default and set
> change values for anything that changed. "Update" would detect what has
changed
> by holding onto the previous values, in this case, the load state
> (api::tabs::TabStatus) and a URL:
> 
> scoped_ptr<DictionaryValue> changed(new DictionaryValue);
> if (status_ == 'loading' && !contents->IsLoading()) {
>   // load status has changed from 'loading' to 'complete'.
>   changed->SetString('status', status_);
>   entry->tab_status = 'complete';
> }
> GURL new_url = contents->GetCommittedURL();
> if (url_ != new_url) {
>   // URL has changed. This may be because the loading state changed,
>   // or it may be an in-page navigation like a #-navigation or
>   // history.push/replaceState.
>   changed->SetString('url', new_url);
>   url_ = new_url;
> }
> // Then later, you or I or somebody else can add:
> std::string new_title = ...;
> if (title_ -> new_title) {
>   changed->SetString('title', new_title);
>   title_ = new_title;
> }
> return changed.Pass();
> 
> Does that make sense? Would it work? Sorry for this big change!

Agree. I tried to have "Update" method instead of 3 method, but it's impossible.
because "UpdateLoadState" is called from TabChangedAt which is called
frequently.

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/tabs/on_updated/historyPushState/a.html
(right):

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/tabs/on_updated/historyPushState/a.html:1:
<!--
On 2015/07/01 15:57:15, kalman wrote:
> This new test isn't a copy, even though git thinks it is, and it will be a
> confusing revision history if it's committed that way.
> It's easy to fix: when you upload the CL, use the "--no-find-copies" flag.

Done. Good tip. Thanks!

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/tabs/on_updated/test.js (right):

https://codereview.chromium.org/1156043002/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/tabs/on_updated/test.js:170: }
On 2015/07/01 15:57:15, kalman wrote:
> Could you also test simple hash fragments, like
location.replace/assign('#foo')?

Done.

https://codereview.chromium.org/1156043002/diff/600001/chrome/browser/extensi...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/1156043002/diff/600001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:85: if
(!complete_waiting_on_load_ && contents->IsLoading()) {
'loading' is not included if previous status is 'loading'.

The only true change is here. (other parts are just style fix and tests)

[not at google - send to devlin, 2015-07-06 19:33:19]

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:428:
changed_properties.reset(entry->UpdateUrl(contents));
On 2015/07/06 17:14:44, limasdf wrote:
> On 2015/07/01 15:57:15, kalman wrote:
> > It's actually not clear to me - or it wouldn't be outside the context of
this
> CL
> > - why a NONE status implies the URL was updated. In other words, why the
> > *current* tab status (which in the case of NONE is very general) implies
> > something about the *change to the URL* (which is very specific).
> > 
> > For example - take the bug about the title changing -
> > https://code.google.com/p/chromium/issues/detail?id=458839. How would that
fit
> > in here? You'd need to pass the |status| in as NONE but then how do you know
> if
> > it's a URL change, versus a title change?
> > 
> > I think the problem is that we're trying to distinguish between 2 different
> > kinds of updates, where really we shouldn't. Instead of those 3 methods
> > DidNavigate/UpdateLoadState/UpdateUrl we should just have an "Update" method
> and
> > a "Reset" method. "Reset" would reset all the state to its default and set
> > change values for anything that changed. "Update" would detect what has
> changed
> > by holding onto the previous values, in this case, the load state
> > (api::tabs::TabStatus) and a URL:
> > 
> > scoped_ptr<DictionaryValue> changed(new DictionaryValue);
> > if (status_ == 'loading' && !contents->IsLoading()) {
> >   // load status has changed from 'loading' to 'complete'.
> >   changed->SetString('status', status_);
> >   entry->tab_status = 'complete';
> > }
> > GURL new_url = contents->GetCommittedURL();
> > if (url_ != new_url) {
> >   // URL has changed. This may be because the loading state changed,
> >   // or it may be an in-page navigation like a #-navigation or
> >   // history.push/replaceState.
> >   changed->SetString('url', new_url);
> >   url_ = new_url;
> > }
> > // Then later, you or I or somebody else can add:
> > std::string new_title = ...;
> > if (title_ -> new_title) {
> >   changed->SetString('title', new_title);
> >   title_ = new_title;
> > }
> > return changed.Pass();
> > 
> > Does that make sense? Would it work? Sorry for this big change!
> 
> Agree. I tried to have "Update" method instead of 3 method, but it's
impossible.
> because "UpdateLoadState" is called from TabChangedAt which is called
> frequently.

Why does that make it impossible?

[not at google - send to devlin, 2015-07-06 19:36:16]

On 2015/07/06 17:14:44, limasdf wrote:
> when you have time -
> 
> Sorry I think this CL doesn't looks good.
> But it would mean the world to us if you leave some comments with wisdom.
> 
> 1. Tests look good?
>  - in the tests, in-page navigations(including history APIs) generate
'loading'
> twice, 'complete' twice. On crbug.com/465709, the bug reporter said it's
wrong.

They should just generate what the change is. Generating "loading" twice would
be wrong, because generating it for a second time implies it didn't change. If
the URL changed, then that's what the change info should say.

Ditto "complete". 4 changes is ok, but saying that the status changed when it
didn't would be a bug.

> 
> 2. Interestingly, on http://Youtube.com, 
> content::NOTIFICATION_NAV_ENTRY_COMMITTED comes again
> with |webcontents->IsLoading() == true| before the completion of page loading.
> this is weird. Only http://Youtube.com acts like that. So this CL just ignore
the
> second contents::NOTIFICATION_NAV_ENTRY_COMMITED.

Is that for the same (top level) frame, or is it because youtube has other
frames? Is there any other information you get (i.e. other changed properties;
is_in_page set, that sort of thing).

[limasdf, 2015-07-21 18:06:47]

Patchset #4 (id:620001) has been deleted

[limasdf, 2015-07-21 18:07:08]

Patchset #4 (id:640001) has been deleted

[limasdf, 2015-07-21 18:15:22]

Patchset #4 (id:660001) has been deleted

[limasdf, 2015-07-21 18:28:55]

Sorry. I have struggled to make single Update() function. but failed. we might
need to keep old style?

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:428:
changed_properties.reset(entry->UpdateUrl(contents));
On 2015/07/06 19:33:19, kalman wrote:
> On 2015/07/06 17:14:44, limasdf wrote:
> > On 2015/07/01 15:57:15, kalman wrote:
> > > It's actually not clear to me - or it wouldn't be outside the context of
> this
> > CL
> > > - why a NONE status implies the URL was updated. In other words, why the
> > > *current* tab status (which in the case of NONE is very general) implies
> > > something about the *change to the URL* (which is very specific).
> > > 
> > > For example - take the bug about the title changing -
> > > https://code.google.com/p/chromium/issues/detail?id=458839. How would that
> fit
> > > in here? You'd need to pass the |status| in as NONE but then how do you
know
> > if
> > > it's a URL change, versus a title change?
> > > 
> > > I think the problem is that we're trying to distinguish between 2
different
> > > kinds of updates, where really we shouldn't. Instead of those 3 methods
> > > DidNavigate/UpdateLoadState/UpdateUrl we should just have an "Update"
method
> > and
> > > a "Reset" method. "Reset" would reset all the state to its default and set
> > > change values for anything that changed. "Update" would detect what has
> > changed
> > > by holding onto the previous values, in this case, the load state
> > > (api::tabs::TabStatus) and a URL:
> > > 
> > > scoped_ptr<DictionaryValue> changed(new DictionaryValue);
> > > if (status_ == 'loading' && !contents->IsLoading()) {
> > >   // load status has changed from 'loading' to 'complete'.
> > >   changed->SetString('status', status_);
> > >   entry->tab_status = 'complete';
> > > }
> > > GURL new_url = contents->GetCommittedURL();
> > > if (url_ != new_url) {
> > >   // URL has changed. This may be because the loading state changed,
> > >   // or it may be an in-page navigation like a #-navigation or
> > >   // history.push/replaceState.
> > >   changed->SetString('url', new_url);
> > >   url_ = new_url;
> > > }
> > > // Then later, you or I or somebody else can add:
> > > std::string new_title = ...;
> > > if (title_ -> new_title) {
> > >   changed->SetString('title', new_title);
> > >   title_ = new_title;
> > > }
> > > return changed.Pass();
> > > 
> > > Does that make sense? Would it work? Sorry for this big change!
> > 
> > Agree. I tried to have "Update" method instead of 3 method, but it's
> impossible.
> > because "UpdateLoadState" is called from TabChangedAt which is called
> > frequently.
> 
> Why does that make it impossible?

I tried to make single "Update" method(See patch set #4). 'complete' and 'url'
are possible. but 'loading' is difficult.

I have tried below

condition#1 :  if (status_ != "loading" && contents->IsLoading() ) 
 - FAIL. because IsLoading is changed frequently. that result in  loading,
complete, loading, complete. (example: chrome://newtab)

condition#2:  if (url_ != contents->GetCommittedURL() && status_ != "loading" &&
contents->IsLoading() ) 
 - FAIL. refresh(F5) doesn't fire onUpdate.

[not at google - send to devlin, 2015-07-21 20:46:42]

On 2015/07/21 18:28:55, limasdf wrote:
> Sorry. I have struggled to make single Update() function. but failed. we might
> need to keep old style?
> 
>
https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
> File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):
> 
>
https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
> chrome/browser/extensions/api/tabs/tabs_event_router.cc:428:
> changed_properties.reset(entry->UpdateUrl(contents));
> On 2015/07/06 19:33:19, kalman wrote:
> > On 2015/07/06 17:14:44, limasdf wrote:
> > > On 2015/07/01 15:57:15, kalman wrote:
> > > > It's actually not clear to me - or it wouldn't be outside the context of
> > this
> > > CL
> > > > - why a NONE status implies the URL was updated. In other words, why the
> > > > *current* tab status (which in the case of NONE is very general) implies
> > > > something about the *change to the URL* (which is very specific).
> > > > 
> > > > For example - take the bug about the title changing -
> > > > https://code.google.com/p/chromium/issues/detail?id=458839. How would
that
> > fit
> > > > in here? You'd need to pass the |status| in as NONE but then how do you
> know
> > > if
> > > > it's a URL change, versus a title change?
> > > > 
> > > > I think the problem is that we're trying to distinguish between 2
> different
> > > > kinds of updates, where really we shouldn't. Instead of those 3 methods
> > > > DidNavigate/UpdateLoadState/UpdateUrl we should just have an "Update"
> method
> > > and
> > > > a "Reset" method. "Reset" would reset all the state to its default and
set
> > > > change values for anything that changed. "Update" would detect what has
> > > changed
> > > > by holding onto the previous values, in this case, the load state
> > > > (api::tabs::TabStatus) and a URL:
> > > > 
> > > > scoped_ptr<DictionaryValue> changed(new DictionaryValue);
> > > > if (status_ == 'loading' && !contents->IsLoading()) {
> > > >   // load status has changed from 'loading' to 'complete'.
> > > >   changed->SetString('status', status_);
> > > >   entry->tab_status = 'complete';
> > > > }
> > > > GURL new_url = contents->GetCommittedURL();
> > > > if (url_ != new_url) {
> > > >   // URL has changed. This may be because the loading state changed,
> > > >   // or it may be an in-page navigation like a #-navigation or
> > > >   // history.push/replaceState.
> > > >   changed->SetString('url', new_url);
> > > >   url_ = new_url;
> > > > }
> > > > // Then later, you or I or somebody else can add:
> > > > std::string new_title = ...;
> > > > if (title_ -> new_title) {
> > > >   changed->SetString('title', new_title);
> > > >   title_ = new_title;
> > > > }
> > > > return changed.Pass();
> > > > 
> > > > Does that make sense? Would it work? Sorry for this big change!
> > > 
> > > Agree. I tried to have "Update" method instead of 3 method, but it's
> > impossible.
> > > because "UpdateLoadState" is called from TabChangedAt which is called
> > > frequently.
> > 
> > Why does that make it impossible?
> 
> I tried to make single "Update" method(See patch set #4). 'complete' and 'url'
> are possible. but 'loading' is difficult.
> 
> I have tried below
> 
> condition#1 :  if (status_ != "loading" && contents->IsLoading() ) 
>  - FAIL. because IsLoading is changed frequently. that result in  loading,
> complete, loading, complete. (example: chrome://newtab)
> 
> condition#2:  if (url_ != contents->GetCommittedURL() && status_ != "loading"
&&
> contents->IsLoading() ) 
>  - FAIL. refresh(F5) doesn't fire onUpdate.

What about if you have example.com open (or whatever), then press F5? Does that
go from complete -> loading? Did it before?

[limasdf, 2015-07-22 05:34:32]

On 2015/07/21 20:46:42, kalman wrote:
> On 2015/07/21 18:28:55, limasdf wrote:
> > Sorry. I have struggled to make single Update() function. but failed. we
might
> > need to keep old style?
> > 
> >
>
https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
> > File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):
> > 
> >
>
https://codereview.chromium.org/1156043002/diff/240001/chrome/browser/extensi...
> > chrome/browser/extensions/api/tabs/tabs_event_router.cc:428:
> > changed_properties.reset(entry->UpdateUrl(contents));
> > On 2015/07/06 19:33:19, kalman wrote:
> > > On 2015/07/06 17:14:44, limasdf wrote:
> > > > On 2015/07/01 15:57:15, kalman wrote:
> > > > > It's actually not clear to me - or it wouldn't be outside the context
of
> > > this
> > > > CL
> > > > > - why a NONE status implies the URL was updated. In other words, why
the
> > > > > *current* tab status (which in the case of NONE is very general)
implies
> > > > > something about the *change to the URL* (which is very specific).
> > > > > 
> > > > > For example - take the bug about the title changing -
> > > > > https://code.google.com/p/chromium/issues/detail?id=458839. How would
> that
> > > fit
> > > > > in here? You'd need to pass the |status| in as NONE but then how do
you
> > know
> > > > if
> > > > > it's a URL change, versus a title change?
> > > > > 
> > > > > I think the problem is that we're trying to distinguish between 2
> > different
> > > > > kinds of updates, where really we shouldn't. Instead of those 3
methods
> > > > > DidNavigate/UpdateLoadState/UpdateUrl we should just have an "Update"
> > method
> > > > and
> > > > > a "Reset" method. "Reset" would reset all the state to its default and
> set
> > > > > change values for anything that changed. "Update" would detect what
has
> > > > changed
> > > > > by holding onto the previous values, in this case, the load state
> > > > > (api::tabs::TabStatus) and a URL:
> > > > > 
> > > > > scoped_ptr<DictionaryValue> changed(new DictionaryValue);
> > > > > if (status_ == 'loading' && !contents->IsLoading()) {
> > > > >   // load status has changed from 'loading' to 'complete'.
> > > > >   changed->SetString('status', status_);
> > > > >   entry->tab_status = 'complete';
> > > > > }
> > > > > GURL new_url = contents->GetCommittedURL();
> > > > > if (url_ != new_url) {
> > > > >   // URL has changed. This may be because the loading state changed,
> > > > >   // or it may be an in-page navigation like a #-navigation or
> > > > >   // history.push/replaceState.
> > > > >   changed->SetString('url', new_url);
> > > > >   url_ = new_url;
> > > > > }
> > > > > // Then later, you or I or somebody else can add:
> > > > > std::string new_title = ...;
> > > > > if (title_ -> new_title) {
> > > > >   changed->SetString('title', new_title);
> > > > >   title_ = new_title;
> > > > > }
> > > > > return changed.Pass();
> > > > > 
> > > > > Does that make sense? Would it work? Sorry for this big change!
> > > > 
> > > > Agree. I tried to have "Update" method instead of 3 method, but it's
> > > impossible.
> > > > because "UpdateLoadState" is called from TabChangedAt which is called
> > > > frequently.
> > > 
> > > Why does that make it impossible?
> > 
> > I tried to make single "Update" method(See patch set #4). 'complete' and
'url'
> > are possible. but 'loading' is difficult.
> > 
> > I have tried below
> > 
> > condition#1 :  if (status_ != "loading" && contents->IsLoading() ) 
> >  - FAIL. because IsLoading is changed frequently. that result in  loading,
> > complete, loading, complete. (example: chrome://newtab)
> > 
> > condition#2:  if (url_ != contents->GetCommittedURL() && status_ !=
"loading"
> &&
> > contents->IsLoading() ) 
> >  - FAIL. refresh(F5) doesn't fire onUpdate.
> 
> What about if you have http://example.com open (or whatever), then press F5?
Does that
> go from complete -> loading? Did it before?

when pressing F5. my patch set #4 does *NOT* go from complete -> loading.
on Chrome 43, it goes from complete -> loading

I think the root problem is WebContents::IsLoading() return value could be
changed several time when loading single web page.
(even with chrome://newtab)

[not at google - send to devlin, 2015-07-22 16:27:58]

> > What about if you have http://example.com open (or whatever), then press F5?
> Does that
> > go from complete -> loading? Did it before?
> 
> when pressing F5. my patch set #4 does *NOT* go from complete -> loading.
> on Chrome 43, it goes from complete -> loading
> 
> I think the root problem is WebContents::IsLoading() return value could be
> changed several time when loading single web page.
> (even with chrome://newtab)

Could the chrome://newtab be redirects?

It's not clear to me what the correct behavior should be - I guess we should
just aim for backwards compatibility, and if that means not ever reporting a
complete --> loading change then so be it (though I think it would be more
correct to report the change).