Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2690903005: Metadata extraction for Copyless Paste (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by wychen
Modified:
2 months, 1 week ago
CC:
abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, kinuko+watch
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Extract JSON-LD metadata for Copyless Paste Copyless paste is a feature which provides a 0-state suggestion for text input, based on the user’s recent context. We'd like to augment the signal by adding structured metadata to it besides just URL and the title. BUG=691765 Review-Url: https://codereview.chromium.org/2690903005 Cr-Commit-Position: refs/heads/master@{#451160} Committed: https://chromium.googlesource.com/chromium/src/+/3b69be515fabeb3652f74ccf02c26c62bdd88bfb

Patch Set 1 #

Total comments: 10

Patch Set 2 : address comments #

Total comments: 5

Patch Set 3 : address dcheng's comments #

Total comments: 8

Patch Set 4 : address esprehn's comments #

Patch Set 5 : rename to document_metadata #

Total comments: 2

Patch Set 6 : histogram #

Unified diffs Side-by-side diffs Delta from patch set Stats (+224 lines, -0 lines) Patch
M third_party/WebKit/Source/modules/BUILD.gn View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/document_metadata/BUILD.gn View 1 2 3 4 1 chunk +14 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.h View 1 2 3 4 1 chunk +25 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp View 1 2 3 4 1 chunk +65 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractorTest.cpp View 1 2 3 4 1 chunk +109 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 1 chunk +9 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 55 (26 generated)
wychen
Would you mind taking a look? Thanks!
2 months, 1 week ago (2017-02-13 21:59:03 UTC) #4
esprehn
Can we move the entire copyless paste feature into blink instead and just use mojo ...
2 months, 1 week ago (2017-02-13 23:47:04 UTC) #9
wychen
Thanks for the prompt review! Quick reply first. https://codereview.chromium.org/2690903005/diff/20001/third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp File third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2690903005/diff/20001/third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp#newcode25 third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp:25: if ...
2 months, 1 week ago (2017-02-14 00:26:08 UTC) #12
esprehn
No I mean something is going to call this method, that should just be code ...
2 months, 1 week ago (2017-02-14 00:30:22 UTC) #13
wychen
On 2017/02/14 00:30:22, esprehn wrote: > No I mean something is going to call this ...
2 months, 1 week ago (2017-02-14 00:40:39 UTC) #14
esprehn
I think inside LocalFrame::create() you'll want to use interfaceRegistry() to add a per frame service ...
2 months, 1 week ago (2017-02-14 00:48:32 UTC) #16
wychen
On 2017/02/14 00:48:32, esprehn wrote: > I think inside LocalFrame::create() you'll want to use interfaceRegistry() ...
2 months, 1 week ago (2017-02-14 01:52:56 UTC) #17
wychen
https://codereview.chromium.org/2690903005/diff/20001/third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp File third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2690903005/diff/20001/third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp#newcode28 third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp:28: element.hasAttribute(HTMLNames::typeAttr) && On 2017/02/13 23:47:04, esprehn wrote: > no ...
2 months, 1 week ago (2017-02-14 02:24:16 UTC) #18
wychen
How does this CL look as is? This CL only introduces the extraction logic. In ...
2 months, 1 week ago (2017-02-14 17:03:56 UTC) #19
dcheng (OOO through May 2)
1) What is copyless paste? 2) If there are any more questions about Mojo, happy ...
2 months, 1 week ago (2017-02-14 19:32:54 UTC) #20
wychen
Updated the description as well. https://codereview.chromium.org/2690903005/diff/40001/third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractorTest.cpp File third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractorTest.cpp (right): https://codereview.chromium.org/2690903005/diff/40001/third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractorTest.cpp#newcode21 third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractorTest.cpp:21: kContent = On 2017/02/14 ...
2 months, 1 week ago (2017-02-14 22:45:49 UTC) #23
esprehn
This is looking pretty good, thanks for all of the iteration. https://codereview.chromium.org/2690903005/diff/60001/third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp File third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp (right): ...
2 months, 1 week ago (2017-02-15 00:10:03 UTC) #24
dglazkov
When I see the name CopylessPasteExtractor, I am confused, because I can't guess the purpose ...
2 months, 1 week ago (2017-02-15 00:23:41 UTC) #25
wychen
https://codereview.chromium.org/2690903005/diff/60001/third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp File third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2690903005/diff/60001/third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp#newcode27 third_party/WebKit/Source/modules/copyless_paste/CopylessPasteExtractor.cpp:27: if (element.hasTagName(HTMLNames::scriptTag) && On 2017/02/15 00:10:03, esprehn wrote: > ...
2 months, 1 week ago (2017-02-15 01:09:33 UTC) #26
wychen
On 2017/02/15 00:23:41, dglazkov wrote: > When I see the name CopylessPasteExtractor, I am confused, ...
2 months, 1 week ago (2017-02-15 01:18:03 UTC) #27
dglazkov
On 2017/02/15 at 01:18:03, wychen wrote: > On 2017/02/15 00:23:41, dglazkov wrote: > > When ...
2 months, 1 week ago (2017-02-15 01:38:26 UTC) #28
esprehn
Maybe we should create a document_metadata module and put both of them there?
2 months, 1 week ago (2017-02-15 01:45:43 UTC) #29
wychen
On 2017/02/15 01:45:43, esprehn wrote: > Maybe we should create a document_metadata module and put ...
2 months, 1 week ago (2017-02-15 01:49:19 UTC) #30
dglazkov
On 2017/02/15 at 01:49:19, wychen wrote: > On 2017/02/15 01:45:43, esprehn wrote: > > Maybe ...
2 months, 1 week ago (2017-02-15 01:55:50 UTC) #31
wychen
On 2017/02/15 01:55:50, dglazkov wrote: > On 2017/02/15 at 01:49:19, wychen wrote: > > On ...
2 months, 1 week ago (2017-02-15 16:49:38 UTC) #32
wychen
It's renamed.
2 months, 1 week ago (2017-02-15 17:40:52 UTC) #33
dglazkov
lgtm
2 months, 1 week ago (2017-02-16 02:05:14 UTC) #34
haraken
(I haven't caught up the long discussion here, so sorry if this is already answered ...
2 months, 1 week ago (2017-02-16 02:33:36 UTC) #38
wychen
+jwd@ for histograms. https://codereview.chromium.org/2690903005/diff/100001/third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2690903005/diff/100001/third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp#newcode60 third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:60: ("CopylessPaste.ExtractionUs", 1, 1000000, 50)); On 2017/02/16 ...
2 months, 1 week ago (2017-02-16 17:29:30 UTC) #42
jwd
histograms lgtm
2 months, 1 week ago (2017-02-16 18:12:03 UTC) #43
wychen
On 2017/02/16 02:33:36, haraken wrote: > (I haven't caught up the long discussion here, so ...
2 months, 1 week ago (2017-02-16 18:28:27 UTC) #44
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690903005/120001
2 months, 1 week ago (2017-02-16 23:34:49 UTC) #51
haraken
On 2017/02/16 18:28:27, wychen wrote: > On 2017/02/16 02:33:36, haraken wrote: > > (I haven't ...
2 months, 1 week ago (2017-02-16 23:37:13 UTC) #52
commit-bot: I haz the power
2 months, 1 week ago (2017-02-17 00:53:37 UTC) #55
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3b69be515fabeb3652f74ccf02c2...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46