Load offline page if reading list entry takes more than 1s to load.

Load offline page if reading list entry takes more than 1s to load.

BUG=None

[jif-google, 2016-09-28 19:45:42]

Description was changed from

==========
Load offline page after 1s.

BUG=None
==========

to

==========
Load offline page after 1s.

BUG=None
==========

[jif-google, 2016-09-28 19:45:42]

jif@google.com changed reviewers:
+ gambard@chromium.org

[jif-google, 2016-09-28 19:45:53]

Patchset #1 (id:1) has been deleted

[jif-google, 2016-09-28 19:46:02]

Patchset #1 (id:20001) has been deleted

[jif-google, 2016-09-28 19:46:08]

Patchset #1 (id:40001) has been deleted

[jif-google, 2016-09-28 19:49:08]

Patchset #1 (id:60001) has been deleted

[jif-google, 2016-09-28 19:49:39]

Description was changed from

==========
Load offline page after 1s.

BUG=None
==========

to

==========
Load offline page if reading list entry takes more than 1s to load.

BUG=None
==========

[jif, 2016-09-28 19:50:53]

Ignore changes outside of ios/

ptal.

[gambard, 2016-10-03 09:04:45]

You will conflict with https://codereview.chromium.org/2369303002/

https://codereview.chromium.org/2378123002/diff/80001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2378123002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:75: auto it =
std::find_if(
Use std::find with a entry created for the find.

https://codereview.chromium.org/2378123002/diff/80001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_web_state_observer.h (right):

https://codereview.chromium.org/2378123002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_web_state_observer.h:22: static
void CreateForWebState(web::WebState* web_state,
Comment?

https://codereview.chromium.org/2378123002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_web_state_observer.h:32: //
WebContentsObserver implementation.
Are you sure it is WebContentsObserver? You do not inherit from it.

https://codereview.chromium.org/2378123002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_web_state_observer.h:33: void
NavigationItemChanged() override;
Those methods are public in WebStateObserver

https://codereview.chromium.org/2378123002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_web_state_observer.h:42: private:
2 "private:"?

https://codereview.chromium.org/2378123002/diff/80001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right):

https://codereview.chromium.org/2378123002/diff/80001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:62:
timer_.reset(new base::Timer(false, true));
Do we want to initialize it with |is_repeating=true|?

[sdefresne, 2016-10-04 13:26:45]

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

[sdefresne, 2016-10-04 13:26:47]

drive-by

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_download_service.cc:99: if (entry)
Can this be null? It look like it iterate over all entries and remove them.
Should this instead be DCHECK(entry)?

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_download_service.cc:105: if (entry)
ditto

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_model.h (right):

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_model.h:56: // Returns a specific
entry. Returns nullptr if the entry does not exist.
s/nullptr/null/ in comments

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_web_state_observer.h (right):

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_web_state_observer.h:23:
web::BrowserState* browser_state);
You should pass a ios::ChromeBrowserState, only code in ios/web and BSKSF should
use web::BrowserState. Or better, since you only pass this to get the
ReadingListModel, just pass a ReadingListModel (this will make the code easier
to unit test).

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_web_state_observer.h:28: explicit
ReadingListWebStateObserver(web::WebState* web_state,
"explicit" is for constructor with one parameter only (or with default values),
remove it from here.

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right):

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:40:
ReadingListModelFactory::GetForBrowserState(chrome_browser_state);
Pass the reading_list_model as a parameter, and remove the browser_state.

https://codereview.chromium.org/2378123002/diff/100001/ios/chrome/browser/rea...
ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:94: if
(progress < 15) {
Can you use a named constant instead of "15" so that someone reading the code
understand what is checked here?

https://codereview.chromium.org/2378123002/diff/100001/ios/web/navigation/nav...
File ios/web/navigation/navigation_item_impl.h (right):

https://codereview.chromium.org/2378123002/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_item_impl.h:73:
base::scoped_nsobject<NSDictionary> extra_data_;
style: ivar should go after all methods, and should not be public.

https://codereview.chromium.org/2378123002/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_item_impl.h:75: 
Remove blank line.

https://codereview.chromium.org/2378123002/diff/100001/ios/web/navigation/nav...
ios/web/navigation/navigation_item_impl.h:148: 
Remove blank line.