move annotations to canvas virtual

move annotations to canvas virtual

In an effort to do it all at once, this change assumes that its ok to ignore annotations that were previously stored on paints in old SKP files (since this feature is only interesting to PDF printing).

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

Committed: https://skia.googlesource.com/skia/+/0eda2587cc9233066cb3f3fec08f35c061780f8e

[reed1, 2016-02-28 22:22:16]

Description was changed from

==========
move annotations to canvas virtual

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

to

==========
move annotations to canvas virtual

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

[reed1, 2016-02-29 00:37:00]

reed@google.com changed reviewers:
+ fmalita@chromium.org, halcanary@google.com

[reed1, 2016-02-29 00:37:55]

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

[commit-bot: I haz the power, 2016-02-29 00:38:06]

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

[reed1, 2016-02-29 00:38:46]

Description was changed from

==========
move annotations to canvas virtual

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

to

==========
move annotations to canvas virtual

In an effort to do it all at once, this change assumes that its ok to ignore
annotations that were previously stored on paints in old SKP files (since this
feature is only interesting to PDF printing).

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

[commit-bot: I haz the power, 2016-02-29 00:40:54]

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

[commit-bot: I haz the power, 2016-02-29 00:40:55]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2016-02-29 03:30:52]

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

[commit-bot: I haz the power, 2016-02-29 03:30:58]

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

[commit-bot: I haz the power, 2016-02-29 03:39:54]

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

[commit-bot: I haz the power, 2016-02-29 03:39:55]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-02-29 03:45:10]

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

[commit-bot: I haz the power, 2016-02-29 03:45:15]

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

[reed1, 2016-02-29 03:45:43]

ptal

[commit-bot: I haz the power, 2016-02-29 03:53:44]

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

[commit-bot: I haz the power, 2016-02-29 03:53:45]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-02-29 04:02:48]

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

[commit-bot: I haz the power, 2016-02-29 04:02:51]

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

[commit-bot: I haz the power, 2016-02-29 04:11:42]

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

[commit-bot: I haz the power, 2016-02-29 04:11:42]

Dry run: This issue passed the CQ dry run.

[f(malita), 2016-02-29 13:53:20]

LGTM

Maybe add an SkDebugger intercept too, although I'm not sure how useful that is.

Ultimately I'd like to see annotations retired from the general API.  My rough
plan is to plumb some sort of printing context all the way to Blink, and then
funnel annotations directly to the SkPDF device.

[f(malita), 2016-02-29 13:54:56]

(BTW, I see some paint.getAnnotation() in a Blink unit test)

[reed1, 2016-02-29 14:13:22]

On 2016/02/29 13:53:20, f(malita) wrote:
> LGTM
> 
> Maybe add an SkDebugger intercept too, although I'm not sure how useful that
is.
> 
> Ultimately I'd like to see annotations retired from the general API.  My rough
> plan is to plumb some sort of printing context all the way to Blink, and then
> funnel annotations directly to the SkPDF device.

Interesting idea, in which case we wouldn't need it on canvas either? I presume
you'd add it to SkDocument?

[f(malita), 2016-02-29 14:19:08]

On 2016/02/29 14:13:22, reed1 wrote:
> On 2016/02/29 13:53:20, f(malita) wrote:
> > LGTM
> > 
> > Maybe add an SkDebugger intercept too, although I'm not sure how useful that
> is.
> > 
> > Ultimately I'd like to see annotations retired from the general API.  My
rough
> > plan is to plumb some sort of printing context all the way to Blink, and
then
> > funnel annotations directly to the SkPDF device.
> 
> Interesting idea, in which case we wouldn't need it on canvas either? I
presume
> you'd add it to SkDocument?

Yeah, no need for it on SkCanvas and SkDocument sounds like a better place.

[hal.canary, 2016-02-29 14:20:39]

On 2016/02/29 at 03:45:43, reed wrote:
> ptal

I approve of this CL.  As I said earlier, I'll test this myself before I 1gtm
it.

[hal.canary, 2016-02-29 14:53:31]

https://codereview.chromium.org/1744103002/diff/80001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/1744103002/diff/80001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1086: void drawAnnotation(const SkRect&, const char
key[], SkData* value);
I propose this:

    enum class AnnotationKey {
        kURL,
        kDefineNamedDestination,
        kLinkNamedDestination,
    };
    void drawAnnotation(const SkRect&, SkCanvas::AnnotationKey, SkData* value);

[hal.canary, 2016-02-29 17:43:01]

I also propose these changes to the headers:

src/core/SkAnnotationKeys.h:

    #ifndef SkAnnotationKeys_DEFINED
    #define SkAnnotationKeys_DEFINED
    /**
     *  Collection of predefined Keys into the Annotation dictionary
     */
    namespace SkAnnotationKeys {
        /** Returns the key whose payload is a URL */
        const char* URL_Key();
        /** Returns the key whose payload is the name of a destination to be
defined. */
        const char* Define_Named_Dest_Key();
        /** Returns the key whose payload is the name of a destination to be
linked to. */
        const char* Link_Named_Dest_Key();
    }
    #endif  // SkAnnotationKeys_DEFINED

include/core/SkAnnotation.h:

    #ifndef SkAnnotation_DEFINED
    #define SkAnnotation_DEFINED
    #include "SkTypes.h"

    class SkCanvas;
    class SkData;
    struct SkPoint;
    struct SkRect;

    /**
     *  Annotate the canvas by associating the specified URL with the
     *  specified rectangle (in local coordinates, just like drawRect). If the
     *  backend of this canvas does not support annotations, this call is
     *  safely ignored.
     *
     *  The caller is responsible for managing its ownership of the SkData.
     */
    SK_API void SkAnnotateRectWithURL(SkCanvas*, const SkRect&, SkData*);

    /**
     *  Annotate the canvas by associating a name with the specified point.
     *
     *  If the backend of this canvas does not support annotations, this call is
     *  safely ignored.
     *
     *  The caller is responsible for managing its ownership of the SkData.
     */
    SK_API void SkAnnotateNamedDestination(SkCanvas*, const SkPoint&, SkData*);

    /**
     *  Annotate the canvas by making the specified rectangle link to a named
     *  destination.
     *
     *  If the backend of this canvas does not support annotations, this call is
     *  safely ignored.
     *
     *  The caller is responsible for managing its ownership of the SkData.
     */
    SK_API void SkAnnotateLinkToDestination(SkCanvas*, const SkRect&, SkData*);
    #endif

[hal.canary, 2016-02-29 20:22:16]

The pdf code lgtm.

Tested inside and outside of chrome.

I can now clean up the PDF code more in a later CL.

Please remove the SkAnnotation class.

[hal.canary, 2016-02-29 20:23:09]

On 2016/02/29 at 14:19:08, fmalita wrote:
> On 2016/02/29 14:13:22, reed1 wrote:
> > On 2016/02/29 13:53:20, f(malita) wrote:
> > > LGTM
> > > 
> > > Maybe add an SkDebugger intercept too, although I'm not sure how useful
that
> > is.
> > > 
> > > Ultimately I'd like to see annotations retired from the general API.  My
rough
> > > plan is to plumb some sort of printing context all the way to Blink, and
then
> > > funnel annotations directly to the SkPDF device.
> > 
> > Interesting idea, in which case we wouldn't need it on canvas either? I
presume
> > you'd add it to SkDocument?
> 
> Yeah, no need for it on SkCanvas and SkDocument sounds like a better place.

This would require a lot of refactoring inside Blink, which is now not aware of
SkDocument.

[reed1, 2016-03-03 15:01:33]

https://codereview.chromium.org/1744103002/diff/80001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/1744103002/diff/80001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1086: void drawAnnotation(const SkRect&, const char
key[], SkData* value);
On 2016/02/29 14:53:31, Hal Canary wrote:
> I propose this:
> 
>     enum class AnnotationKey {
>         kURL,
>         kDefineNamedDestination,
>         kLinkNamedDestination,
>     };
>     void drawAnnotation(const SkRect&, SkCanvas::AnnotationKey, SkData*
value);

Possibly, but I'd like to be conservative for the moment, and just focus on
moving from paint to canvas. Using strings may prove valuable if we want to
reused these for other purposes (e.g. comments).

[reed1, 2016-03-03 15:01:48]

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

[commit-bot: I haz the power, 2016-03-03 15:01:51]

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

[reed1, 2016-03-03 15:02:13]

On 2016/02/29 17:43:01, Hal Canary wrote:
> I also propose these changes to the headers:
> 
> src/core/SkAnnotationKeys.h:
> 
>     #ifndef SkAnnotationKeys_DEFINED
>     #define SkAnnotationKeys_DEFINED
>     /**
>      *  Collection of predefined Keys into the Annotation dictionary
>      */
>     namespace SkAnnotationKeys {
>         /** Returns the key whose payload is a URL */
>         const char* URL_Key();
>         /** Returns the key whose payload is the name of a destination to be
> defined. */
>         const char* Define_Named_Dest_Key();
>         /** Returns the key whose payload is the name of a destination to be
> linked to. */
>         const char* Link_Named_Dest_Key();
>     }
>     #endif  // SkAnnotationKeys_DEFINED
> 
> include/core/SkAnnotation.h:
> 
>     #ifndef SkAnnotation_DEFINED
>     #define SkAnnotation_DEFINED
>     #include "SkTypes.h"
> 
>     class SkCanvas;
>     class SkData;
>     struct SkPoint;
>     struct SkRect;
> 
>     /**
>      *  Annotate the canvas by associating the specified URL with the
>      *  specified rectangle (in local coordinates, just like drawRect). If the
>      *  backend of this canvas does not support annotations, this call is
>      *  safely ignored.
>      *
>      *  The caller is responsible for managing its ownership of the SkData.
>      */
>     SK_API void SkAnnotateRectWithURL(SkCanvas*, const SkRect&, SkData*);
> 
>     /**
>      *  Annotate the canvas by associating a name with the specified point.
>      *
>      *  If the backend of this canvas does not support annotations, this call
is
>      *  safely ignored.
>      *
>      *  The caller is responsible for managing its ownership of the SkData.
>      */
>     SK_API void SkAnnotateNamedDestination(SkCanvas*, const SkPoint&,
SkData*);
> 
>     /**
>      *  Annotate the canvas by making the specified rectangle link to a named
>      *  destination.
>      *
>      *  If the backend of this canvas does not support annotations, this call
is
>      *  safely ignored.
>      *
>      *  The caller is responsible for managing its ownership of the SkData.
>      */
>     SK_API void SkAnnotateLinkToDestination(SkCanvas*, const SkRect&,
SkData*);
>     #endif

Done

[commit-bot: I haz the power, 2016-03-03 15:04:48]

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

[commit-bot: I haz the power, 2016-03-03 15:04:49]

Dry run: Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[hal.canary, 2016-03-03 15:11:49]

perfect

[hal.canary, 2016-03-03 15:12:22]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2016-03-03 15:12:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
halcanary@google.com, fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1744103002/#ps120001
(title: "update dox")

[commit-bot: I haz the power, 2016-03-03 15:12:32]

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

[commit-bot: I haz the power, 2016-03-03 15:15:03]

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

[commit-bot: I haz the power, 2016-03-03 15:15:05]

Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[reed1, 2016-03-03 16:01:29]

The CQ bit was checked by reed@google.com

[reed1, 2016-03-03 16:01:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
halcanary@google.com, fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1744103002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-03-03 16:01:43]

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

[commit-bot: I haz the power, 2016-03-03 16:14:00]

Description was changed from

==========
move annotations to canvas virtual

In an effort to do it all at once, this change assumes that its ok to ignore
annotations that were previously stored on paints in old SKP files (since this
feature is only interesting to PDF printing).

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

to

==========
move annotations to canvas virtual

In an effort to do it all at once, this change assumes that its ok to ignore
annotations that were previously stored on paints in old SKP files (since this
feature is only interesting to PDF printing).

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

Committed:
https://skia.googlesource.com/skia/+/0eda2587cc9233066cb3f3fec08f35c061780f8e
==========

[commit-bot: I haz the power, 2016-03-03 16:14:01]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/0eda2587cc9233066cb3f3fec08f35c061780f8e

[reed1, 2016-03-03 17:14:20]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1761793003/ by reed@google.com.

The reason for reverting is: need to update unittest in blink:

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/third_party/WebKit/Source/core/page/webkit_unit_tests.PrintContextTest.o.d
-DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2
-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DCHROMIUM_BUILD
-DCR_CLANG_REVISION=261368-1 -DCOMPONENT_BUILD -DUSE_LIBJPEG_TURBO=1
-DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DENABLE_PEPPER_CDMS
-DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_TOPCHROME_MD=1
-DDCHECK_ALWAYS_ON=1 -DFIELDTRIAL_TESTING_ENABLED -DENABLE_TASK_MANAGER=1
-DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1
-DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD
-DSAFE_BROWSING_DB_LOCAL -DBLINK_IMPLEMENTATION=1 -DINSIDE_BLINK
-DMOJO_USE_SYSTEM_IMPL -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=0 -DSKIA_DLL
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_SUPPORT_GPU=1
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG
-DENABLE_LAYOUT_UNIT_IN_INLINE_BOXES=0
-DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1
-DWTF_USE_ICCJPEG=1 -DWTF_USE_QCMSLIB=1 -DENABLE_OILPAN=1 -DUNIT_TEST
-DGTEST_HAS_RTTI=0 -DV8_SHARED -DUSING_V8_SHARED -DUSE_LIBPCI=1 -DUSE_OPENSSL=1
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -Igen -I../../third_party/WebKit/public/web
-I../../third_party/WebKit/Source/web -I../../third_party/WebKit/Source/web/src
-I../../third_party/WebKit/public/web/mac -I../.. -I../../skia/config
-I../../third_party/WebKit/Source -I../../third_party/khronos -I../../gpu
-Igen/angle -I../../third_party/WebKit -I../../skia/ext
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils
-I../../third_party/skia/include/utils/mac -I../../third_party/icu/source/common
-I../../third_party/npapi -I../../third_party/npapi/bindings
-I../../third_party/libpng -I../../third_party/ots/include
-I../../third_party/qcms/src -I../../third_party/iccjpeg
-I../../third_party/libjpeg_turbo -I../../third_party/WebKit
-I../../third_party/icu/source/i18n -I../../testing/gmock/include
-I../../testing/gtest/include -I../../third_party/libwebp
-I../../third_party/zlib -I../../v8/include -Igen/blink -isysroot
/Applications/Xcode5.1.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk
-O0 -fvisibility=hidden -Werror -mmacosx-version-min=10.6 -arch x86_64 -Wall
-Wextra -Wno-unused-parameter -Wno-missing-field-initializers
-Wno-selector-type-mismatch -Wpartial-availability -Wheader-hygiene
-Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wno-inconsistent-missing-override
-Wno-shift-negative-value -Wexit-time-destructors -std=c++11 -stdlib=libc++
-fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics
-Xclang -load -Xclang
/b/build/slave/mac/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -Xclang
-plugin-arg-find-bad-constructs -Xclang follow-macro-expansion
-fcolor-diagnostics -fno-strict-aliasing -Xclang -load -Xclang
/b/build/slave/mac/build/src/third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib
-Xclang -add-plugin -Xclang blink-gc-plugin -Xclang -plugin-arg-blink-gc-plugin
-Xclang enable-oilpan -Xclang -plugin-arg-blink-gc-plugin -Xclang warn-raw-ptr
-fstack-protector-all  -c
../../third_party/WebKit/Source/core/page/PrintContextTest.cpp -o
obj/third_party/WebKit/Source/core/page/webkit_unit_tests.PrintContextTest.o
../../third_party/WebKit/Source/core/page/PrintContextTest.cpp:54:20: error: no
member named 'getAnnotation' in 'SkPaint'
        if (!paint.getAnnotation())
             ~~~~~ ^
.