Move common distiller code to superclass

Move common distiller code to superclass

This change moves any code related to JavaScript and HTML generation
in dom distiller to a superclass that all platforms can use.

BUG=472797
Committed: https://crrev.com/b9fee24969e63f6b172d62c6a26c56812cb3b697
Cr-Commit-Position: refs/heads/master@{#327055}

[mdjones, 2015-04-24 16:02:22]

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

[commit-bot: I haz the power, 2015-04-24 16:04:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1101993003/20001

[commit-bot: I haz the power, 2015-04-24 16:04:02]

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

[commit-bot: I haz the power, 2015-04-24 16:04:02]

Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mdjones, 2015-04-24 16:28:44]

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

[nyquist, 2015-04-24 22:13:09]

https://codereview.chromium.org/1101993003/diff/20001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_request_view_base.h (right):

https://codereview.chromium.org/1101993003/diff/20001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:30: virtual void
runCallback(std::string& data) = 0;
could the param be const?

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

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller.gypi:85:
'dom_distiller/core/dom_distiller_request_view_base.cc',
Do you need to update GN?

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:42: void
runCallback(std::string& data) override;
RunCallback

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:68: explicit
RequestViewerHandle(content::WebContents* web_contents,
Does this need to be explicit?

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:95: void
OnChangeFontFamily(
Could these observer methods also move up to the base class? I think they end up
just calling SendJavaScript.

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_request_view_base.h (right):

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:26: // (namely
IOS and android) which makes this necessary.
Nit: iOS and Android

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:30: virtual void
runCallback(std::string& data) = 0;
RunCallback. Also, could the param be const?

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:44: void
flagAsErrorPage();
FlagAsErrorPage

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:55: // ready.
Also, maybe add the sanity check comment of: "When the viewer becomes ready, the
buffered data should be flushed" or something along those lines?

I wonder though, is that something this base class could help out with possibly?
Maybe having some virtual method "IsReady()" which would be called, and later
make the subclasses call "OnReady()", which would flush the buffer? I'm not sure
it's worth it though. Might just be confusing. Any thoughts?

Secondly, maybe mention here, that the buffers are self contained (and
terminating JavaScript code), so it should be safe to append to an existing
buffer?

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:63: //
content::URLDataSource::GotDataCallback callback_;
Remove?

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:74: bool
is_error_page_;
Is this ever read anywhere?

[mdjones, 2015-04-25 00:55:03]

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

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller.gypi:85:
'dom_distiller/core/dom_distiller_request_view_base.cc',
On 2015/04/24 22:13:08, nyquist wrote:
> Do you need to update GN?

Done.

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:42: void
runCallback(std::string& data) override;
On 2015/04/24 22:13:09, nyquist wrote:
> RunCallback

Done.

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:68: explicit
RequestViewerHandle(content::WebContents* web_contents,
On 2015/04/24 22:13:08, nyquist wrote:
> Does this need to be explicit?

Not as far as I can tell.

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:95: void
OnChangeFontFamily(
On 2015/04/24 22:13:09, nyquist wrote:
> Could these observer methods also move up to the base class? I think they end
up
> just calling SendJavaScript.

They can. Originally I left them out because iOS does not use them, but it makes
sense to have them here. Done

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_request_view_base.h (right):

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:26: // (namely
IOS and android) which makes this necessary.
On 2015/04/24 22:13:09, nyquist wrote:
> Nit: iOS and Android

Done.

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:30: virtual void
runCallback(std::string& data) = 0;
On 2015/04/24 22:13:09, nyquist wrote:
> RunCallback. Also, could the param be const?

Fixed function name; I could make this const but it would require me making a
copy of the string to make a RefCountedString.

This whole callback class concept may not actually be around too long once early
template loading lands. That requires some work on IOS though.

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:44: void
flagAsErrorPage();
On 2015/04/24 22:13:09, nyquist wrote:
> FlagAsErrorPage

Done.

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:55: // ready.
On 2015/04/24 22:13:09, nyquist wrote:
> Also, maybe add the sanity check comment of: "When the viewer becomes ready,
the
> buffered data should be flushed" or something along those lines?
> 
> I wonder though, is that something this base class could help out with
possibly?
> Maybe having some virtual method "IsReady()" which would be called, and later
> make the subclasses call "OnReady()", which would flush the buffer? I'm not
sure
> it's worth it though. Might just be confusing. Any thoughts?
> 
> Secondly, maybe mention here, that the buffers are self contained (and
> terminating JavaScript code), so it should be safe to append to an existing
> buffer?

There are a few things that need to be worked out on IOS before this makes
sense. For example, I would need the WebContents in the IOS version and I am
unsure of how difficult that is.

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:63: //
content::URLDataSource::GotDataCallback callback_;
On 2015/04/24 22:13:09, nyquist wrote:
> Remove?

Done.

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:74: bool
is_error_page_;
On 2015/04/24 22:13:09, nyquist wrote:
> Is this ever read anywhere?

Made a proper getter (IsErrorPage) for this which is read in viewer source.

[nyquist, 2015-04-25 03:15:56]

lgtm

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_request_view_base.h (right):

https://codereview.chromium.org/1101993003/diff/40001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.h:55: // ready.
On 2015/04/25 00:55:03, mdjones wrote:
> On 2015/04/24 22:13:09, nyquist wrote:
> > Also, maybe add the sanity check comment of: "When the viewer becomes ready,
> the
> > buffered data should be flushed" or something along those lines?
> > 
> > I wonder though, is that something this base class could help out with
> possibly?
> > Maybe having some virtual method "IsReady()" which would be called, and
later
> > make the subclasses call "OnReady()", which would flush the buffer? I'm not
> sure
> > it's worth it though. Might just be confusing. Any thoughts?
> > 
> > Secondly, maybe mention here, that the buffers are self contained (and
> > terminating JavaScript code), so it should be safe to append to an existing
> > buffer?
> 
> There are a few things that need to be worked out on IOS before this makes
> sense. For example, I would need the WebContents in the IOS version and I am
> unsure of how difficult that is.

Yeah. iOS doesn't have WebContents. I was thinking about having one method for
sending the JS in the subclass liked now, but having this base class buffer the
JS. However the subclass's would then have to tell this parent class to flush
the buffer once the viewer was ready. And this base class would have to be able
to call a method in the subclass to see whether the viewer was ready to receive
JS or not.

Anyway, no point of doing this in this refactoring CL.

[mdjones, 2015-04-27 15:38:24]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2015-04-27 15:38:30]

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

[commit-bot: I haz the power, 2015-04-27 16:41:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-27 16:41:52]

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