Delete the temporary file when generating MHTML with the extension API.

Delete the temporary file when generating MHTML with the extension API.

We now delete the temporary file created when generating MHTML from the
extension API.

BUG=97489
TEST=The MHTML extension API should still work.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111180

[Mihai Parparita -not on Chrome, 2011-11-15 18:51:42]

Is it possible to add a test for this (that the temp file is deleted)? See
ExtensionTabsTest for the new hotness for extension tests.

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.h:290: class
ExtensionRenderViewHostObserver : public RenderViewHostObserver {
Does this have to live in ExtensionUIFunction (instead of just in your function
subclass)? UpdateTabFunction was able to use TabContentsObserver directly.

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_save_page_api.cc (right):

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_save_page_api.cc:174: Browser* browser =
NULL;
Body of this function is indented one too many times.

[asargent_no_longer_on_chrome, 2011-11-15 18:56:30]

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function.cc (right):

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.cc:55: // Default behavior is to
delete this.
This comment is kind of confusing - do you mean that the default behavor of the
RenderViewHostDestroyed function from the superclass is to delete |this|?

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.h:290: class
ExtensionRenderViewHostObserver : public RenderViewHostObserver {
We already have the RenderViewHostTracker class defined above for watching the
lifetime of RVH's. Did you consider just adding this functionality to that
class?

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_save_page_api.cc (right):

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_save_page_api.cc:169: // Note that we'll
wait for a response ack message before we call Release()
nit: might be worth mentioning that where this happens is in
OnMessageReceivedFromRenderView

[Jay Civelli, 2011-11-18 23:14:32]

@sky: could you review the change to ui_test_utils?

@mihai and antony:
I added the test that the file gets deleted to the existing test. Exposing the
gc method did the trick.

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function.cc (right):

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.cc:55: // Default behavior is to
delete this.
On 2011/11/15 18:56:31, Antony Sargent wrote:
> This comment is kind of confusing - do you mean that the default behavor of
the
> RenderViewHostDestroyed function from the superclass is to delete |this|?
> 
Yes. Clarified comment.

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.h:290: class
ExtensionRenderViewHostObserver : public RenderViewHostObserver {
On 2011/11/15 18:51:42, Mihai Parparita wrote:
> Does this have to live in ExtensionUIFunction (instead of just in your
function
> subclass)? UpdateTabFunction was able to use TabContentsObserver directly.
My case is actually different. The TabContentsObserver won't receive that
message, because it is coming from the extension popup render view, not the tab
contents ( ExecuteCodeInTabFunction and UpdateTabFunction talk to the tab's
render view). That's why I have to listen directly at the RenderViewHost level.

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function.h:290: class
ExtensionRenderViewHostObserver : public RenderViewHostObserver {
On 2011/11/15 18:56:31, Antony Sargent wrote:
> We already have the RenderViewHostTracker class defined above for watching the
> lifetime of RVH's. Did you consider just adding this functionality to that
> class? 
Good idea, done.

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_save_page_api.cc (right):

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_save_page_api.cc:169: // Note that we'll
wait for a response ack message before we call Release()
On 2011/11/15 18:56:31, Antony Sargent wrote:
> nit: might be worth mentioning that where this happens is in
> OnMessageReceivedFromRenderView

Done.

http://codereview.chromium.org/8530003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_save_page_api.cc:174: Browser* browser =
NULL;
On 2011/11/15 18:51:42, Mihai Parparita wrote:
> Body of this function is indented one too many times.

Done.

[sky, 2011-11-18 23:24:56]

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.cc (right):

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:232:
MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
Can you use RunAllPendingInMessageLoop again here? You'll likely need to change
RunMessageLoop to deal with a non-ui thread, but that seems like a good thing
anyway.

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:281: 
nit: remove line.

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.h (right):

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.h:87: // Blocks the current thread until all the
pending messages in the  loop of the
nit: 'the  loop' -> 'the loop'

[Jay Civelli, 2011-11-19 00:01:12]

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.cc (right):

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:232:
MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
On 2011/11/18 23:24:56, sky wrote:
> Can you use RunAllPendingInMessageLoop again here? You'll likely need to
change
> RunMessageLoop to deal with a non-ui thread, but that seems like a good thing
> anyway.

Done.

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:281: 
On 2011/11/18 23:24:56, sky wrote:
> nit: remove line.

Done.

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.h (right):

http://codereview.chromium.org/8530003/diff/15002/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.h:87: // Blocks the current thread until all the
pending messages in the  loop of the
On 2011/11/18 23:24:56, sky wrote:
> nit: 'the  loop' -> 'the loop'

Done.

[sky, 2011-11-19 00:07:08]

LGTM

[Mihai Parparita -not on Chrome, 2011-11-19 01:25:20]

LGTM

http://codereview.chromium.org/8530003/diff/20002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_save_page_api.cc (right):

http://codereview.chromium.org/8530003/diff/20002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_save_page_api.cc:100: 
Extra newline.

http://codereview.chromium.org/8530003/diff/20002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_save_page_api.h (right):

http://codereview.chromium.org/8530003/diff/20002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_save_page_api.h:61: static TestDelegate*
test_delegate_;
This can just be a static inside exension_save_page_api.cc

[Jay Civelli, 2011-11-21 02:12:48]

@jam, could you review the 1 added line in content/renderer/mhtml_generator.cc?
We were just not closing a file.

http://codereview.chromium.org/8530003/diff/20002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_save_page_api.cc (right):

http://codereview.chromium.org/8530003/diff/20002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_save_page_api.cc:100: 
On 2011/11/19 01:25:21, Mihai Parparita wrote:
> Extra newline.

Done.

http://codereview.chromium.org/8530003/diff/20002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_save_page_api.h (right):

http://codereview.chromium.org/8530003/diff/20002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_save_page_api.h:61: static TestDelegate*
test_delegate_;
On 2011/11/19 01:25:21, Mihai Parparita wrote:
> This can just be a static inside exension_save_page_api.cc

Done.

[asargent_no_longer_on_chrome, 2011-11-21 17:43:35]

lgtm

[jam, 2011-11-21 22:07:17]

content lgtm

[commit-bot: I haz the power, 2011-11-21 22:09:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/8530003/33001

[commit-bot: I haz the power, 2011-11-22 00:20:27]

Try job failure for 8530003-33001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "net_unittests, unit_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-22 02:02:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/8530003/33001

[commit-bot: I haz the power, 2011-11-22 02:02:31]

Can't apply patch for file chrome/browser/extensions/extension_function.h.
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_function.h
Hunk #2 FAILED at 213.
Hunk #3 succeeded at 277 (offset 10 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_function.h.rej

[commit-bot: I haz the power, 2011-11-22 08:33:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/8530003/37009

[Aaron Boodman, 2011-11-22 09:23:52]

http://codereview.chromium.org/8530003/diff/37009/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/schema_generated_bindings.js (right):

http://codereview.chromium.org/8530003/diff/37009/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/schema_generated_bindings.js:804:
SendResponseAck(request.id);
Did you consider putting a "response_needs_ack" or something in the json_schema
file, so that it is slightly more general ? I worry that future callers will
just copy this code and/or put custom bindings in so they can get the same
effect.

[commit-bot: I haz the power, 2011-11-22 09:42:03]

List of reviewers changed. aa@chromium.org did a drive-by without LGTM'ing!

[jcivelli, 2011-11-22 15:41:56]

On 2011/11/22 09:42:03, I haz the power (commit-bot) wrote:
> List of reviewers changed. mailto:aa@chromium.org did a drive-by without
LGTM'ing!

@aaron
Do you mind if I investigate this as a follow up CL? I have been desperately
trying to last this CL and have several other changes in the pipe depending on
this one.

[Aaron Boodman, 2011-11-22 17:17:08]

LGTM

[commit-bot: I haz the power, 2011-11-22 17:21:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jcivelli@chromium.org/8530003/37009

[commit-bot: I haz the power, 2011-11-22 18:35:01]

Change committed as 111180