Refactor MHTMLGenerator to allow getting sending the data to a file descriptor.

Refactor MHTMLGenerator to allow getting sending the data to a file descriptor.

TBR=darin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220059

[qsr, 2013-08-23 10:09:19]

This change is a precursor work for a feature I'm working on that will save
MHTML into a cache. Having to pass through an intermediary file is, in that
case, a waste of resources.

[qsr, 2013-08-28 09:12:19]

Gentle ping?

[mkosiba (inactive), 2013-08-28 09:34:48]

Jay is no longer on the Chromium project. LGTM (I don't have OWNERS for
everything though).

BTW, it seems a bit scary that there aren't any unittests for this. If you're
going to make more changes to the code it migt be a good idea to add some?

[qsr, 2013-08-28 09:39:29]

On 2013/08/28 09:34:48, mkosiba wrote:
> Jay is no longer on the Chromium project. LGTM (I don't have OWNERS for
> everything though).
> 
> BTW, it seems a bit scary that there aren't any unittests for this. If you're
> going to make more changes to the code it migt be a good idea to add some?

 Not a bad idea :) Will look at it.

[qsr, 2013-08-28 09:40:11]

+TBR darin for owner approval.

[commit-bot: I haz the power, 2013-08-28 09:42:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/22920005/1

[mkosiba (inactive), 2013-08-28 10:34:36]

https://chromiumcodereview.appspot.com/22920005/diff/1/android_webview/native...
File android_webview/native/aw_contents.cc (right):

https://chromiumcodereview.appspot.com/22920005/diff/1/android_webview/native...
android_webview/native/aw_contents.cc:308: base::Bind(&GenerateMHTMLCallback,
path, base::Owned(j_callback)));
looks like this should be taget_path instead of path

[commit-bot: I haz the power, 2013-08-28 10:42:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[qsr, 2013-08-28 11:21:32]

https://chromiumcodereview.appspot.com/22920005/diff/1/android_webview/native...
File android_webview/native/aw_contents.cc (right):

https://chromiumcodereview.appspot.com/22920005/diff/1/android_webview/native...
android_webview/native/aw_contents.cc:308: base::Bind(&GenerateMHTMLCallback,
path, base::Owned(j_callback)));
 Yep... Thanks and done.

[commit-bot: I haz the power, 2013-08-28 11:25:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/22920005/30001

[commit-bot: I haz the power, 2013-08-28 12:05:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-28 12:13:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/22920005/47001

[commit-bot: I haz the power, 2013-08-28 12:43:50]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-28 12:52:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/22920005/64001

[commit-bot: I haz the power, 2013-08-28 18:19:51]

Change committed as 220059

[joth, 2013-08-28 19:02:37]

(maybe for a follow up...)

https://chromiumcodereview.appspot.com/22920005/diff/64001/android_webview/na...
File android_webview/native/aw_contents.cc (right):

https://chromiumcodereview.appspot.com/22920005/diff/64001/android_webview/na...
android_webview/native/aw_contents.cc:308: base::Bind(&GenerateMHTMLCallback,
base::Owned(j_callback), target_path));
GenerateMHTMLCallback completely ignores the |path| parameter passed to it so
why not just remove it from the function rather than curry it in here?

[qsr, 2013-08-29 08:45:37]

https://codereview.chromium.org/22920005/diff/64001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/22920005/diff/64001/android_webview/native/aw...
android_webview/native/aw_contents.cc:308: base::Bind(&GenerateMHTMLCallback,
base::Owned(j_callback), target_path));
On 2013/08/28 19:02:37, joth wrote:
> GenerateMHTMLCallback completely ignores the |path| parameter passed to it so
> why not just remove it from the function rather than curry it in here?

 Which GenerateMHTMLCallback are you speaking about? The one just before this
use the path argument. If it's the java one you are speaking about, I did not go
look at it, but looking at this, it does use the path argument.

[joth, 2013-08-29 13:23:45]

https://codereview.chromium.org/22920005/diff/64001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/22920005/diff/64001/android_webview/native/aw...
android_webview/native/aw_contents.cc:308: base::Bind(&GenerateMHTMLCallback,
base::Owned(j_callback), target_path));
On 2013/08/29 08:45:38, qsr wrote:
> On 2013/08/28 19:02:37, joth wrote:
> > GenerateMHTMLCallback completely ignores the |path| parameter passed to it
so
> > why not just remove it from the function rather than curry it in here?
> 
>  Which GenerateMHTMLCallback are you speaking about? The one just before this
> use the path argument. If it's the java one you are speaking about, I did not
go
> look at it, but looking at this, it does use the path argument.

Ah my mistake - I meant the one immediately above but I see it does use it after
all. Sorry for the bother!