Upstream iOS implementation of dom_distiller

Upstream iOS implementation of dom_distiller

Provides an implementation of DistillerPage that use the web API to
inject the distiller JavaScript code into the page.

Introduce an iOS specific implementation of domdistiller.js to work
around a crash in UIWebView when creating a local "window" object.

BUG=459678
Committed: https://crrev.com/27eaf1e3fc955adc0a555844320738cf867cc38b
Cr-Commit-Position: refs/heads/master@{#317813}

[sdefresne, 2015-02-18 19:29:18]

sdefresne@chromium.org changed reviewers:
+ droger@chromium.org, nyquist@chromium.org

[sdefresne, 2015-02-18 19:29:18]

droger: please review the code (this is almost straight-forward port of
downstream code) and the new DEPS

nyquist: can you review as OWNERS of //components/dom_distiller

noyau: FYI as I'm adding you as OWNERS of //components/dom_distiller/ios (since
you know the code and kkhorimoto is not committer)

kkhorimoto: FYI as original author of the downstream version of the code

[noyau (Ping after 24h), 2015-02-19 09:15:42]

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

[noyau (Ping after 24h), 2015-02-19 09:15:43]

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015 :)

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.mm:78: }
Either put the brackets on one liner if, or don't. But please don't mix and
match in the same method.

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
File components/dom_distiller/ios/javascript/domdistiller.js (right):

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/javascript/domdistiller.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2015

[droger, 2015-02-19 09:38:49]

lgtm

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:10: #include
"base/mac/scoped_nsobject.h"
Remove.

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:16: class GURL;
You need to actually include gurl.h

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:31: // Loads urls and injects
JavaScript into a page, extracting the distilled page
s/urls/URLs/

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.mm:94:
weak_this->HandleJavaScriptResultString(string);
You need to return early if weak_this is null, or don't use a weak pointer.

[sdefresne, 2015-02-19 10:27:32]

New patchsets have been uploaded after l-g-t-m from droger@chromium.org

[sdefresne, 2015-02-19 10:50:13]

PTAL

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/19 at 09:15:43, noyau wrote:
> 2015 :)

We generally don't change the copyright line when upstreaming/moving files.

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:10: #include
"base/mac/scoped_nsobject.h"
On 2015/02/19 at 09:38:49, droger wrote:
> Remove.

Done.

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:16: class GURL;
On 2015/02/19 at 09:38:49, droger wrote:
> You need to actually include gurl.h

Done.

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:31: // Loads urls and injects
JavaScript into a page, extracting the distilled page
On 2015/02/19 at 09:38:49, droger wrote:
> s/urls/URLs/

Done.

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.mm:78: }
On 2015/02/19 at 09:15:43, noyau wrote:
> Either put the brackets on one liner if, or don't. But please don't mix and
match in the same method.

Done.

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.mm:94:
weak_this->HandleJavaScriptResultString(string);
On 2015/02/19 at 09:38:49, droger wrote:
> You need to return early if weak_this is null, or don't use a weak pointer.

Good catch, done.

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
File components/dom_distiller/ios/javascript/domdistiller.js (right):

https://codereview.chromium.org/936843002/diff/20001/components/dom_distiller...
components/dom_distiller/ios/javascript/domdistiller.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/19 at 09:15:43, noyau wrote:
> 2015

Same remark about year.

[sdefresne, 2015-02-19 10:50:20]

PTAL

[noyau (Ping after 24h), 2015-02-19 10:57:13]

lgtm

[sdefresne, 2015-02-20 10:29:52]

sdefresne@chromium.org changed reviewers:
+ bengr@chromium.org
- nyquist@chromium.org

[sdefresne, 2015-02-20 10:29:52]

bengr: could you review components/dom_distiller as OWNERS?

[nyquist, 2015-02-20 17:40:41]

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

[nyquist, 2015-02-20 18:55:48]

lgtm

By the way, we are hoping to move the JS into the external repo. We might have
to do something special for iOS there naturally.
https://codereview.chromium.org/901793002/

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller.gypi (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller.gypi:190:
'../ios/provider/ios_provider_web.gyp:ios_provider_web',
Nit: Could you move this after local reps?

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller.gypi:192: 'dom_distiller_core',
Not: Alphabetical ordering of local deps

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Not: 2015

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/javascript/domdistiller.js (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/javascript/domdistiller.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Not: 2015

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/javascript/domdistiller.js:7: (function() {
For core I believe the folder name is just 'js'. Any reason why this is
different?

[bengr, 2015-02-20 20:11:38]

nits. lgtm.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_factory_ios.h (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_factory_ios.h:19: class
DistillerPageFactoryIOS : public DistillerPageFactory {
Add a class comment.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_factory_ios.h:24: const gfx::Size&
view_size) const override;
Add blank line below.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_factory_ios.mm (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_factory_ios.mm:19: return
scoped_ptr<DistillerPage>(new DistillerPageIOS(browser_state_));
Use make_scoped_ptr

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:40: friend class
DistillerWebStateObserver;
Add blank line below.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:42: void
OnLoadURLDone(web::PageLoadCompletionStatus load_completion_status);
Add blank line below.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.mm:46: #pragma mark -
What is this doing?

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.mm:103:
static_cast<base::DictionaryValue*>(
Is this safe?

[sdefresne, 2015-02-23 11:07:25]

New patchsets have been uploaded after l-g-t-m from
noyau@chromium.org,nyquist@chromium.org,bengr@chromium.org

[sdefresne, 2015-02-23 11:08:58]

Thank you for the review.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller.gypi (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller.gypi:190:
'../ios/provider/ios_provider_web.gyp:ios_provider_web',
On 2015/02/20 at 18:55:48, nyquist wrote:
> Nit: Could you move this after local reps?

I've sorted the deps alphabetically as suggested, and this put the non-local
before. Other dependencies block also put non-local first in this file, so I'll
leave it here.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller.gypi:192: 'dom_distiller_core',
On 2015/02/20 at 18:55:48, nyquist wrote:
> Not: Alphabetical ordering of local deps

Ooops, done.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_factory_ios.h (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_factory_ios.h:19: class
DistillerPageFactoryIOS : public DistillerPageFactory {
On 2015/02/20 at 20:11:38, bengr wrote:
> Add a class comment.

Done.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_factory_ios.h:24: const gfx::Size&
view_size) const override;
On 2015/02/20 at 20:11:37, bengr wrote:
> Add blank line below.

This is the list of method overridden from DistillerPageFactory interface, the
common style throughout the Chromium code base seems to put a comment on top "//
Implementation of DistillerPageFactory" and put all the overridden methods
without blank lines between them.

Added the comment.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_factory_ios.mm (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_factory_ios.mm:19: return
scoped_ptr<DistillerPage>(new DistillerPageIOS(browser_state_));
On 2015/02/20 at 20:11:38, bengr wrote:
> Use make_scoped_ptr

I don't really see the benefit here (since the types are different), but done.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.mm:46: #pragma mark -
On 2015/02/20 at 20:11:38, bengr wrote:
> What is this doing?

This allow grouping the method in Xcode IDE. This is heavily used by the iOS
team.

You can read more about it here: http://nshipster.com/pragma/

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.mm:103:
static_cast<base::DictionaryValue*>(
On 2015/02/20 at 20:11:38, bengr wrote:
> Is this safe?

Should be, but I've changed it to check the value type so that it is safe.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/javascript/domdistiller.js (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/javascript/domdistiller.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/20 at 18:55:48, nyquist wrote:
> Not: 2015

Code was written in 2014, only upstreaming happened in 2015. In general, we do
not change the copyright notice when upstreaming (moving from private repo to
public repo).

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/javascript/domdistiller.js:7: (function() {
On 2015/02/20 at 18:55:48, nyquist wrote:
> For core I believe the folder name is just 'js'. Any reason why this is
different?

The code file is located at
"components/dom_distiller/core/javascript/domdistiller.js", so this is
consitent.

[sdefresne, 2015-02-23 13:16:26]

noyau: I've removed unused #include and unused dependencies, could you take
another quick look?

[noyau (Ping after 24h), 2015-02-23 13:22:51]

lgtm again. Note that you missed two comments by bengr about missing empty
lines.

[sdefresne, 2015-02-23 14:47:48]

New patchsets have been uploaded after l-g-t-m from noyau@chromium.org

[sdefresne, 2015-02-23 14:48:12]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-02-23 14:48:13]

Thank you.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:40: friend class
DistillerWebStateObserver;
On 2015/02/20 at 20:11:38, bengr wrote:
> Add blank line below.

Done.

https://codereview.chromium.org/936843002/diff/40001/components/dom_distiller...
components/dom_distiller/ios/distiller_page_ios.h:42: void
OnLoadURLDone(web::PageLoadCompletionStatus load_completion_status);
On 2015/02/20 at 20:11:38, bengr wrote:
> Add blank line below.

Done.

[commit-bot: I haz the power, 2015-02-23 14:48:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/936843002/140001

[commit-bot: I haz the power, 2015-02-23 14:50:52]

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

[commit-bot: I haz the power, 2015-02-23 14:50:54]

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

[sdefresne, 2015-02-24 17:03:01]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-02-24 17:03:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/936843002/140001

[commit-bot: I haz the power, 2015-02-24 17:19:21]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-24 17:20:01]

Patchset 8 (id:??) landed as
https://crrev.com/27eaf1e3fc955adc0a555844320738cf867cc38b
Cr-Commit-Position: refs/heads/master@{#317813}