SkPDF: Hold page objects, not SkPDFDevices.

SkPDF: Hold page objects, not SkPDFDevices.

Page Contents:
    serialize early, at endPage()

SkPDFDocument:
   Rather than holding all SkPDFDevices until onClose(), store
   fGlyphUseage and array of pages objects.

perform_font_subsetting function removed:
   First half moved to onEndPage.
   Second half moved to onClose.

create_pdf_page function removed:
   Merged into onEndPage.

generate_page_tree:
   Refactored to use SkTArray<sk_sp<T>> over SkTDArray<T*>.
   (the former is explicit about ownership, the latter is unclear.)

   No longer populates a structure of objects to be dumped,
   unnecessary since dump is always called after serialization.
   Takes ownership of fPages and returns root of tree-ified version.
   Less reference churn.

SkPDFGlyphSetMap:
    use new-style iterator.

BUG=skia:5087
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1823683005

Committed: https://skia.googlesource.com/skia/+/cc77c12293d1685f5e83d768b30ca9157af1576d

[hal.canary, 2016-03-21 21:51:26]

Description was changed from

==========
SkPDF: Hold page objects, not SkPDFDevices.

Page Contents:
    serialize early, at endPage()

SkPDFDocument:
   Rather than holding all SkPDFDevices until onClose(), store
   fGlyphUseage and array of pages objects.

perform_font_subsetting function removed:
   First half moved to onEndPage.
   Second half moved to onClose.

create_pdf_page function removed:
   Merged into onEndPage.

generate_page_tree:
   Refactored to use SkTArray<sk_sp<T>> over SkTDArray<T*>.
   No longer populates a structure of objects to be dumped,
   unnecessary since dump is always called after serialization.
   Takes ownership of fPages and returns root of tree-ified version.
   Less reference churn.

SkPDFGlyphSetMap:
    use new-style iterator.

BUG=skia:5087
==========

to

==========
SkPDF: Hold page objects, not SkPDFDevices.

Page Contents:
    serialize early, at endPage()

SkPDFDocument:
   Rather than holding all SkPDFDevices until onClose(), store
   fGlyphUseage and array of pages objects.

perform_font_subsetting function removed:
   First half moved to onEndPage.
   Second half moved to onClose.

create_pdf_page function removed:
   Merged into onEndPage.

generate_page_tree:
   Refactored to use SkTArray<sk_sp<T>> over SkTDArray<T*>.
   No longer populates a structure of objects to be dumped,
   unnecessary since dump is always called after serialization.
   Takes ownership of fPages and returns root of tree-ified version.
   Less reference churn.

SkPDFGlyphSetMap:
    use new-style iterator.

BUG=skia:5087
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[hal.canary, 2016-03-21 21:52:00]

halcanary@google.com changed reviewers:
+ tomhudson@google.com

[hal.canary, 2016-03-21 21:52:01]

PTAL. This should clean things up a lot.

[tomhudson, 2016-03-22 14:56:17]

Meta-nit: I like seeing more *why* than *what* in the CL description. (Not that
I always remember to do this myself...) Do we prefer using SkTArray over
SkTDArray? Why? (I think that's the received wisdom, but looking at the code for
those classes it isn't obvious why it'd be true. Or since I can't verify it by
looking at the code, maybe I have things backwards.)

https://codereview.chromium.org/1823683005/diff/1/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.chromium.org/1823683005/diff/1/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:130: kids->appendObjRef(std::move(curNodes[i]));
As we work our way through curNodes, it's transiently invalid (because we've
moved these pointers out and left holes in the array), right? You've done a
bunch to optimize this, but is the performance improvement worth it?

https://codereview.chromium.org/1823683005/diff/1/src/pdf/SkPDFDocument.h
File src/pdf/SkPDFDocument.h (right):

https://codereview.chromium.org/1823683005/diff/1/src/pdf/SkPDFDocument.h#new...
src/pdf/SkPDFDocument.h:75: SkPDFGlyphSetMap fGlyphUseage;
sp: fGlyphUsage, not useage

[hal.canary, 2016-03-22 20:56:54]

Description was changed from

==========
SkPDF: Hold page objects, not SkPDFDevices.

Page Contents:
    serialize early, at endPage()

SkPDFDocument:
   Rather than holding all SkPDFDevices until onClose(), store
   fGlyphUseage and array of pages objects.

perform_font_subsetting function removed:
   First half moved to onEndPage.
   Second half moved to onClose.

create_pdf_page function removed:
   Merged into onEndPage.

generate_page_tree:
   Refactored to use SkTArray<sk_sp<T>> over SkTDArray<T*>.
   No longer populates a structure of objects to be dumped,
   unnecessary since dump is always called after serialization.
   Takes ownership of fPages and returns root of tree-ified version.
   Less reference churn.

SkPDFGlyphSetMap:
    use new-style iterator.

BUG=skia:5087
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
SkPDF: Hold page objects, not SkPDFDevices.

Page Contents:
    serialize early, at endPage()

SkPDFDocument:
   Rather than holding all SkPDFDevices until onClose(), store
   fGlyphUseage and array of pages objects.

perform_font_subsetting function removed:
   First half moved to onEndPage.
   Second half moved to onClose.

create_pdf_page function removed:
   Merged into onEndPage.

generate_page_tree:
   Refactored to use SkTArray<sk_sp<T>> over SkTDArray<T*>.
   (the former is explicit about ownership, the latter is unclear.)

   No longer populates a structure of objects to be dumped,
   unnecessary since dump is always called after serialization.
   Takes ownership of fPages and returns root of tree-ified version.
   Less reference churn.

SkPDFGlyphSetMap:
    use new-style iterator.

BUG=skia:5087
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[hal.canary, 2016-03-22 21:17:29]

On 2016/03/22 14:56:17, tomhudson wrote:
> Meta-nit: I like seeing more *why* than *what* in the CL description. (Not
that
> I always remember to do this myself...) Do we prefer using SkTArray over
> SkTDArray? Why? (I think that's the received wisdom, but looking at the code
for
> those classes it isn't obvious why it'd be true. Or since I can't verify it by
> looking at the code, maybe I have things backwards.)

done:
    generate_page_tree:
       Refactored to use SkTArray<sk_sp<T>> over SkTDArray<T*>.
       (the former is explicit about ownership, the latter is unclear.)

[hal.canary, 2016-03-22 21:17:43]

https://codereview.chromium.org/1823683005/diff/1/src/pdf/SkPDFDocument.cpp
File src/pdf/SkPDFDocument.cpp (right):

https://codereview.chromium.org/1823683005/diff/1/src/pdf/SkPDFDocument.cpp#n...
src/pdf/SkPDFDocument.cpp:130: kids->appendObjRef(std::move(curNodes[i]));
On 2016/03/22 14:56:17, tomhudson wrote:
> As we work our way through curNodes, it's transiently invalid (because we've
> moved these pointers out and left holes in the array), right? You've done a
> bunch to optimize this, but is the performance improvement worth it?

Not really.  when we move, we leave  a nullpr behind. (Look at the
implementation of sk_sp(sk_sp<T>&& that).)

I'll add some asserts to show that we never try to use a nullptr. and that we
move every object.

https://codereview.chromium.org/1823683005/diff/1/src/pdf/SkPDFDocument.h
File src/pdf/SkPDFDocument.h (right):

https://codereview.chromium.org/1823683005/diff/1/src/pdf/SkPDFDocument.h#new...
src/pdf/SkPDFDocument.h:75: SkPDFGlyphSetMap fGlyphUseage;
On 2016/03/22 14:56:17, tomhudson wrote:
> sp: fGlyphUsage, not useage

Done.

Why can't I spell?

[hal.canary, 2016-03-22 21:17:54]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-22 21:17:57]

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

[commit-bot: I haz the power, 2016-03-22 21:32:19]

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

[commit-bot: I haz the power, 2016-03-22 21:32:20]

Dry run: This issue passed the CQ dry run.

[tomhudson, 2016-03-23 11:57:37]

lgtm

[hal.canary, 2016-03-23 12:19:17]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2016-03-23 12:19:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1823683005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1823683005/20001

[commit-bot: I haz the power, 2016-03-23 13:26:38]

Description was changed from

==========
SkPDF: Hold page objects, not SkPDFDevices.

Page Contents:
    serialize early, at endPage()

SkPDFDocument:
   Rather than holding all SkPDFDevices until onClose(), store
   fGlyphUseage and array of pages objects.

perform_font_subsetting function removed:
   First half moved to onEndPage.
   Second half moved to onClose.

create_pdf_page function removed:
   Merged into onEndPage.

generate_page_tree:
   Refactored to use SkTArray<sk_sp<T>> over SkTDArray<T*>.
   (the former is explicit about ownership, the latter is unclear.)

   No longer populates a structure of objects to be dumped,
   unnecessary since dump is always called after serialization.
   Takes ownership of fPages and returns root of tree-ified version.
   Less reference churn.

SkPDFGlyphSetMap:
    use new-style iterator.

BUG=skia:5087
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
SkPDF: Hold page objects, not SkPDFDevices.

Page Contents:
    serialize early, at endPage()

SkPDFDocument:
   Rather than holding all SkPDFDevices until onClose(), store
   fGlyphUseage and array of pages objects.

perform_font_subsetting function removed:
   First half moved to onEndPage.
   Second half moved to onClose.

create_pdf_page function removed:
   Merged into onEndPage.

generate_page_tree:
   Refactored to use SkTArray<sk_sp<T>> over SkTDArray<T*>.
   (the former is explicit about ownership, the latter is unclear.)

   No longer populates a structure of objects to be dumped,
   unnecessary since dump is always called after serialization.
   Takes ownership of fPages and returns root of tree-ified version.
   Less reference churn.

SkPDFGlyphSetMap:
    use new-style iterator.

BUG=skia:5087
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/cc77c12293d1685f5e83d768b30ca9157af1576d
==========

[commit-bot: I haz the power, 2016-03-23 13:26:39]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/cc77c12293d1685f5e83d768b30ca9157af1576d