Downloading the json file from ChromeReader is tried several times when ERR_NETW…

Downloading the json file from ChromeReader is tried several times when ERR_NETWORK_CHANGED or 5xx HTTP error occurs.

BUG=593357
Committed: https://crrev.com/174facf59a38990ca2a9594404df601a43756c9d
Cr-Commit-Position: refs/heads/master@{#383701}

[jkrcal, 2016-03-11 13:56:44]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 13:57:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1789513003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1789513003/1

[commit-bot: I haz the power, 2016-03-11 14:05:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 14:05:57]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Marc Treib, 2016-03-23 11:50:07]

treib@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-03-23 11:50:08]

Hey Jan,
I know you didn't actually send this out for review yet, but have some comments
anyway ;)

If you have some spare time, you could try to get this CL ready soon? (You might
want to rebase it off of the other CL, using "git reparent-branch origin/master"
or something like that.)
Patrick regularly complains about missing snippets, and I think this should
help. :)  (If you're busy, don't sweat it, he'll just have to wait a bit
longer!)

Ah, one more nit: Remove "Fixing bug ..." from the title and description, since
it's already in the "BUG=..."

https://codereview.chromium.org/1789513003/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1789513003/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.cc:75: // especially at startup and
after sign-in on ChromeOS. Retrying once should
I would remove everything after the first line here, as it's not really
relevant/applicable in our case.

https://codereview.chromium.org/1789513003/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: // Enable fetching the files
properly even with poor connection
This is IMO a bit too generic - it's more "try to make a bit more robust" rather
than "enable".

[chromium-reviews, 2016-03-23 15:56:33]

Hi Marc,

I have a bit of time today, I'll give it a try.

And thanks for the comments!
Jan

On Wed, Mar 23, 2016 at 4:50 AM <treib@chromium.org> wrote:

> Hey Jan,
> I know you didn't actually send this out for review yet, but have some
> comments
> anyway ;)
>
> If you have some spare time, you could try to get this CL ready soon? (You
> might
> want to rebase it off of the other CL, using "git reparent-branch
> origin/master"
> or something like that.)
> Patrick regularly complains about missing snippets, and I think this should
> help. :) (If you're busy, don't sweat it, he'll just have to wait a bit
> longer!)
>
> Ah, one more nit: Remove "Fixing bug ..." from the title and description,
> since
> it's already in the "BUG=..."
>
>
>
>
https://codereview.chromium.org/1789513003/diff/1/components/ntp_snippets/ntp...
> File components/ntp_snippets/ntp_snippets_fetcher.cc (right):
>
>
>
https://codereview.chromium.org/1789513003/diff/1/components/ntp_snippets/ntp...
> components/ntp_snippets/ntp_snippets_fetcher.cc:75: // especially at
> startup and after sign-in on ChromeOS. Retrying once should
> I would remove everything after the first line here, as it's not really
> relevant/applicable in our case.
>
>
>
https://codereview.chromium.org/1789513003/diff/1/components/ntp_snippets/ntp...
> components/ntp_snippets/ntp_snippets_fetcher.cc:79: // Enable fetching
> the files properly even with poor connection
> This is IMO a bit too generic - it's more "try to make a bit more
> robust" rather than "enable".
>
> https://codereview.chromium.org/1789513003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jkrcal, 2016-03-23 16:18:28]

Description was changed from

==========
Fixing bug 593357, downloading the json file from ChromeReader is tried several
times when ERR_NETWORK_CHANGED or 5xx HTTP error occurs.

BUG=593357
==========

to

==========
Fixing bug 593357, downloading the json file from ChromeReader is tried several
times when ERR_NETWORK_CHANGED or 5xx HTTP error occurs.

BUG=593357
==========

[jkrcal, 2016-03-23 17:14:03]

Hi Marc,

I think I've messed it up :)

The command
git reparent-branch origin/master
seems to have merged the two CLs into a big one.

I tried to fix it but I failed so far. Need to focus on the classes here in MTV.
I guess I'll need your help next Tuesday :|

Jan

> > If you have some spare time, you could try to get this CL ready soon? (You
> > might
> > want to rebase it off of the other CL, using "git reparent-branch
> > origin/master"
> > or something like that.)

[Marc Treib, 2016-03-23 17:21:06]

On 2016/03/23 17:14:03, jkrcal wrote:
> Hi Marc,
> 
> I think I've messed it up :)
> 
> The command
> git reparent-branch origin/master
> seems to have merged the two CLs into a big one.

Woah, that is very weird!
At this point, the easiest way out may be to partially start over:
Delete Patch Set 2 here.
Make a new local branch: git new-branch snippets_retry
Apply the CL: git cl patch 1789513003
Do the review fixes again (annoying, but not a huge deal in this case).
Upload again: git cl upload

Happy to do it together on Tuesday! Don't neglect your classes because of this
:)

> I tried to fix it but I failed so far. Need to focus on the classes here in
MTV.
> I guess I'll need your help next Tuesday :|
> 
> Jan
> 
> > > If you have some spare time, you could try to get this CL ready soon? (You
> > > might
> > > want to rebase it off of the other CL, using "git reparent-branch
> > > origin/master"
> > > or something like that.)

[jkrcal, 2016-03-23 21:00:27]

Patchset #2 (id:20001) has been deleted

[jkrcal, 2016-03-23 21:10:28]

PTAL

https://codereview.chromium.org/1789513003/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_fetcher.cc (right):

https://codereview.chromium.org/1789513003/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.cc:75: // especially at startup and
after sign-in on ChromeOS. Retrying once should
On 2016/03/23 11:50:08, Marc Treib wrote:
> I would remove everything after the first line here, as it's not really
> relevant/applicable in our case.

Done.

https://codereview.chromium.org/1789513003/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_fetcher.cc:79: // Enable fetching the files
properly even with poor connection
On 2016/03/23 11:50:08, Marc Treib wrote:
> This is IMO a bit too generic - it's more "try to make a bit more robust"
rather
> than "enable".

Done.

[Marc Treib, 2016-03-24 08:40:28]

Description was changed from

==========
Fixing bug 593357, downloading the json file from ChromeReader is tried several
times when ERR_NETWORK_CHANGED or 5xx HTTP error occurs.

BUG=593357
==========

to

==========
Downloading the json file from ChromeReader is tried several times when
ERR_NETWORK_CHANGED or 5xx HTTP error occurs.

BUG=593357
==========

[Marc Treib, 2016-03-24 08:40:56]

LGTM, thanks!

[Marc Treib, 2016-03-24 08:41:43]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-03-24 08:41:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1789513003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1789513003/40001

[commit-bot: I haz the power, 2016-03-24 08:43:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-24 08:43:37]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Marc Treib, 2016-03-24 08:49:26]

On 2016/03/24 08:43:37, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
>   ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
>   ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
>   ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

Argh, looks like this needs another rebase... :-/  (as in, "git rebase-update")
Feel free to just tick "Commit" once you've done that, you don't need my lgtm
again :)

[jkrcal, 2016-03-29 10:03:41]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 10:03:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1789513003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1789513003/60001

[commit-bot: I haz the power, 2016-03-29 10:05:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-29 10:05:28]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)

[jkrcal, 2016-03-29 10:33:29]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2016-03-29 10:33:30]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1789513003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-29 10:33:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1789513003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1789513003/80001

[commit-bot: I haz the power, 2016-03-29 11:24:40]

Description was changed from

==========
Downloading the json file from ChromeReader is tried several times when
ERR_NETWORK_CHANGED or 5xx HTTP error occurs.

BUG=593357
==========

to

==========
Downloading the json file from ChromeReader is tried several times when
ERR_NETWORK_CHANGED or 5xx HTTP error occurs.

BUG=593357
==========

[commit-bot: I haz the power, 2016-03-29 11:24:41]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-29 11:25:43]

Description was changed from

==========
Downloading the json file from ChromeReader is tried several times when
ERR_NETWORK_CHANGED or 5xx HTTP error occurs.

BUG=593357
==========

to

==========
Downloading the json file from ChromeReader is tried several times when
ERR_NETWORK_CHANGED or 5xx HTTP error occurs.

BUG=593357

Committed: https://crrev.com/174facf59a38990ca2a9594404df601a43756c9d
Cr-Commit-Position: refs/heads/master@{#383701}
==========

[commit-bot: I haz the power, 2016-03-29 11:25:44]

Patchset 4 (id:??) landed as
https://crrev.com/174facf59a38990ca2a9594404df601a43756c9d
Cr-Commit-Position: refs/heads/master@{#383701}