Clear the PrintContext in WebLocalFrameImpl::Close().

Clear the PrintContext in WebLocalFrameImpl::Close().

Also explicitly call PrintContext::EndPrintMode(), rather than calling
it in the destructor. Add a ScopedPrintMode helper class to do that
conveniently.

BUG=716474
Review-Url: https://codereview.chromium.org/2848823005
Cr-Commit-Position: refs/heads/master@{#468882}
Committed: https://chromium.googlesource.com/chromium/src/+/85b7d7b31f25da2481ab2fa569c7b37a42abdeb6

[Lei Zhang, 2017-04-29 08:43:52]

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

[commit-bot: I haz the power, 2017-04-29 08:44:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 10:56:03]

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

[commit-bot: I haz the power, 2017-04-29 10:56:03]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2017-04-29 16:34:52]

thestig@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-04-29 16:38:46]

This CL looks good, but to make it clearer that EndPrintMode() is never called
for on-heap PrintContexts, can you make the following change?

- Make all call sites explicitly call EndPrintMode().
- Add DCHECK(!is_printing_) to ~PrintContext().

[Lei Zhang, 2017-04-30 21:54:00]

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

[Lei Zhang, 2017-04-30 21:54:12]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-04-30 21:54:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2017-04-30 21:55:12]

Description was changed from

==========
Clear the PrintContext in WebLocalFrameImpl::Close().

BUG=716474
==========

to

==========
Clear the PrintContext in WebLocalFrameImpl::Close().

Also explicitly call PrintContext::EndPrintMode(), rather than calling
it in the destructor.

BUG=716474
==========

[commit-bot: I haz the power, 2017-04-30 22:34:30]

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

[commit-bot: I haz the power, 2017-04-30 22:34:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Lei Zhang, 2017-04-30 22:44:33]

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

[commit-bot: I haz the power, 2017-04-30 22:44:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-04-30 22:44:42]

On 2017/04/29 16:38:46, haraken wrote:
> This CL looks good, but to make it clearer that EndPrintMode() is never called
> for on-heap PrintContexts, can you make the following change?
> 
> - Make all call sites explicitly call EndPrintMode().
> - Add DCHECK(!is_printing_) to ~PrintContext().

FWIW, having to explicitly call this everywhere seems a little error prone,
especially when a function has early returns. Can we make a STACK_ALLOCATED()
helper to do this cleanup where it's possible instead?

[Lei Zhang, 2017-04-30 23:31:14]

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

[commit-bot: I haz the power, 2017-04-30 23:31:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2017-04-30 23:32:18]

On 2017/04/30 22:44:42, dcheng (OOO through May 2) wrote:
> On 2017/04/29 16:38:46, haraken wrote:
> > This CL looks good, but to make it clearer that EndPrintMode() is never
called
> > for on-heap PrintContexts, can you make the following change?
> > 
> > - Make all call sites explicitly call EndPrintMode().
> > - Add DCHECK(!is_printing_) to ~PrintContext().
> 
> FWIW, having to explicitly call this everywhere seems a little error prone,
> especially when a function has early returns. Can we make a STACK_ALLOCATED()
> helper to do this cleanup where it's possible instead?

Added in patch set 4.

[Lei Zhang, 2017-04-30 23:33:25]

Description was changed from

==========
Clear the PrintContext in WebLocalFrameImpl::Close().

Also explicitly call PrintContext::EndPrintMode(), rather than calling
it in the destructor.

BUG=716474
==========

to

==========
Clear the PrintContext in WebLocalFrameImpl::Close().

Also explicitly call PrintContext::EndPrintMode(), rather than calling
it in the destructor. Add a ScopedPrintMode helper class to do that
conveniently.

BUG=716474
==========

[haraken, 2017-05-01 00:21:17]

https://codereview.chromium.org/2848823005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/PrintContext.cpp (right):

https://codereview.chromium.org/2848823005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/PrintContext.cpp:217:
ScopedPrintMode(&print_context, page_rect.Width(), page_rect.Height());

Would it be simpler to create ScopedPrintContext (for most cases) and a
heap-allocated PrintContext (for the LocalFrameImpl case)?

[commit-bot: I haz the power, 2017-05-01 00:43:58]

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

[commit-bot: I haz the power, 2017-05-01 00:43:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Lei Zhang, 2017-05-01 03:16:21]

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

[commit-bot: I haz the power, 2017-05-01 03:16:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2017-05-01 03:18:27]

https://codereview.chromium.org/2848823005/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/PrintContext.cpp (right):

https://codereview.chromium.org/2848823005/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/PrintContext.cpp:217:
ScopedPrintMode(&print_context, page_rect.Width(), page_rect.Height());
On 2017/05/01 00:21:16, haraken wrote:
> 
> Would it be simpler to create ScopedPrintContext (for most cases) and a
> heap-allocated PrintContext (for the LocalFrameImpl case)?

Sure. See patch set 5. I tried it earlier but I did something wrong and the
blink-gc Clang plugin stopped me.

[commit-bot: I haz the power, 2017-05-01 04:54:28]

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

[commit-bot: I haz the power, 2017-05-01 04:54:29]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-05-01 08:11:29]

On 2017/05/01 03:18:27, Lei Zhang (OOO May Day) wrote:
>
https://codereview.chromium.org/2848823005/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/PrintContext.cpp (right):
> 
>
https://codereview.chromium.org/2848823005/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/PrintContext.cpp:217:
> ScopedPrintMode(&print_context, page_rect.Width(), page_rect.Height());
> On 2017/05/01 00:21:16, haraken wrote:
> > 
> > Would it be simpler to create ScopedPrintContext (for most cases) and a
> > heap-allocated PrintContext (for the LocalFrameImpl case)?
> 
> Sure. See patch set 5. I tried it earlier but I did something wrong and the
> blink-gc Clang plugin stopped me.

What error are you hitting? PS5 seems passing all tests.

[Lei Zhang, 2017-05-02 18:33:42]

On 2017/05/01 08:11:29, haraken wrote:
> What error are you hitting? PS5 seems passing all tests.

No errors. I had a local build earlier that didn't work right. Patch set 5 is
fine.

[haraken, 2017-05-03 00:59:54]

LGTM

[Lei Zhang, 2017-05-03 01:27:52]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2017-05-03 01:28:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 03:07:18]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493774872994200,
"parent_rev": "278ed1baed2e3bc863de6bfe2dde19d625f968be", "commit_rev":
"85b7d7b31f25da2481ab2fa569c7b37a42abdeb6"}

[commit-bot: I haz the power, 2017-05-03 03:08:21]

Description was changed from

==========
Clear the PrintContext in WebLocalFrameImpl::Close().

Also explicitly call PrintContext::EndPrintMode(), rather than calling
it in the destructor. Add a ScopedPrintMode helper class to do that
conveniently.

BUG=716474
==========

to

==========
Clear the PrintContext in WebLocalFrameImpl::Close().

Also explicitly call PrintContext::EndPrintMode(), rather than calling
it in the destructor. Add a ScopedPrintMode helper class to do that
conveniently.

BUG=716474

Review-Url: https://codereview.chromium.org/2848823005
Cr-Commit-Position: refs/heads/master@{#468882}
Committed:
https://chromium.googlesource.com/chromium/src/+/85b7d7b31f25da2481ab2fa569c7...
==========

[commit-bot: I haz the power, 2017-05-03 03:08:24]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/85b7d7b31f25da2481ab2fa569c7...