Issue 1265843005: Refactor feedback to use native JS object

Issue 1265843005: Refactor feedback to use native JS object (Closed)

Created:
5 years, 4 months ago by mdjones

Modified:
5 years, 3 months ago

Reviewers:
nyquist, wychen

CC:
chromium-reviews, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, darin (slow to review), ben+mojo_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@js-mojo-combine

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Refactor feedback to use native JS object This change moves the triggering logic for feedback out of DomDistillerViewerSource and into the native "distiller" JavaScript object. BUG= Committed: https://crrev.com/65173ea8d88884cf71b5713165c4c069b9b714dc Cr-Commit-Position: refs/heads/master@{#350574}

Patch Set 1 #

Patch Set 2 : Rebase #

Total comments: 4

Patch Set 3 : Param order #

Total comments: 12

Patch Set 4 : address comments #

Patch Set 5 : impl comments #

Created: 5 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+81 lines, -37 lines)			Patch
M	components/dom_distiller/content/browser/distiller_javascript_service_impl.h	View	1 2 3 4	2 chunks	+13 lines, -0 lines	0 comments	Download
M	components/dom_distiller/content/browser/distiller_javascript_service_impl.cc	View	1 2 3	1 chunk	+30 lines, -3 lines	0 comments	Download
M	components/dom_distiller/content/browser/dom_distiller_viewer_source.cc	View	1	2 chunks	+3 lines, -15 lines	0 comments	Download
M	components/dom_distiller/content/common/distiller_javascript_service.mojom	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download
M	components/dom_distiller/content/renderer/distiller_native_javascript.h	View	1 2 3	1 chunk	+6 lines, -0 lines	0 comments	Download
M	components/dom_distiller/content/renderer/distiller_native_javascript.cc	View	1 2 3	1 chunk	+18 lines, -2 lines	0 comments	Download
M	components/dom_distiller/core/javascript/dom_distiller_viewer.js	View	1	2 chunks	+6 lines, -17 lines	0 comments	Download

Depends on Patchset:

Issue 1231083007 Patch 260001

Messages

Total messages: 13 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

wychen

https://codereview.chromium.org/1265843005/diff/20001/components/dom_distiller/content/browser/distiller_javascript_service_impl.h File components/dom_distiller/content/browser/distiller_javascript_service_impl.h (right): https://codereview.chromium.org/1265843005/diff/20001/components/dom_distiller/content/browser/distiller_javascript_service_impl.h#newcode29 components/dom_distiller/content/browser/distiller_javascript_service_impl.h:29: mojo::StrongBinding<DistillerJavaScriptService> binding_; Where is this used? https://codereview.chromium.org/1265843005/diff/20001/components/dom_distiller/content/browser/distiller_javascript_service_impl.h#newcode35 components/dom_distiller/content/browser/distiller_javascript_service_impl.h:35: void ...

5 years, 3 months ago (2015-09-09 21:12:24 UTC) #3

mdjones

5 years, 3 months ago (2015-09-09 22:35:16 UTC) #4

nyquist

https://codereview.chromium.org/1265843005/diff/40001/components/dom_distiller/content/browser/distiller_javascript_service_impl.cc File components/dom_distiller/content/browser/distiller_javascript_service_impl.cc (right): https://codereview.chromium.org/1265843005/diff/40001/components/dom_distiller/content/browser/distiller_javascript_service_impl.cc#newcode33 components/dom_distiller/content/browser/distiller_javascript_service_impl.cc:33: FeedbackReporter::ReportQuality(false); Could we just do: ### FeedbackReporter::ReportQuality(good); if (good) ...

5 years, 3 months ago (2015-09-10 19:04:13 UTC) #5

mdjones

https://codereview.chromium.org/1265843005/diff/40001/components/dom_distiller/content/browser/distiller_javascript_service_impl.cc File components/dom_distiller/content/browser/distiller_javascript_service_impl.cc (right): https://codereview.chromium.org/1265843005/diff/40001/components/dom_distiller/content/browser/distiller_javascript_service_impl.cc#newcode33 components/dom_distiller/content/browser/distiller_javascript_service_impl.cc:33: FeedbackReporter::ReportQuality(false); On 2015/09/10 19:04:13, nyquist wrote: > Could we ...

5 years, 3 months ago (2015-09-11 16:50:14 UTC) #6

nyquist

https://codereview.chromium.org/1265843005/diff/40001/components/dom_distiller/content/common/distiller_javascript_service.mojom File components/dom_distiller/content/common/distiller_javascript_service.mojom (right): https://codereview.chromium.org/1265843005/diff/40001/components/dom_distiller/content/common/distiller_javascript_service.mojom#newcode11 components/dom_distiller/content/common/distiller_javascript_service.mojom:11: HandleDistillerFeedbackCall(bool good); On 2015/09/11 16:50:13, mdjones wrote: > On ...

5 years, 3 months ago (2015-09-11 17:14:23 UTC) #7

mdjones

5 years, 3 months ago (2015-09-11 18:25:54 UTC) #8

https://codereview.chromium.org/1265843005/diff/40001/components/dom_distille...
File components/dom_distiller/content/common/distiller_javascript_service.mojom
(right):

https://codereview.chromium.org/1265843005/diff/40001/components/dom_distille...
components/dom_distiller/content/common/distiller_javascript_service.mojom:11:
HandleDistillerFeedbackCall(bool good);
On 2015/09/11 17:14:22, nyquist wrote:
> On 2015/09/11 16:50:13, mdjones wrote:
> > On 2015/09/10 19:04:13, nyquist wrote:
> > > Should we have comments for these two methods? Or do you not think it's
> > helpful?
> > 
> > I tried to make the function names self-documenting, but I added comments
for
> > clarity.
> 
> Oh. Right. Yeah, I think the method names are explanatory by themselves. The
new
> comments aren't really more helpful than the method name.
> 
> I was thinking more:
> "Triggers sending user feedback using the system feedback mechanism."

I would expect that kind of comment to be written for the implementation rather
than the interface; I added comments in the .h file for this implementation.

https://codereview.chromium.org/1265843005/diff/40001/components/dom_distille...
File components/dom_distiller/content/renderer/distiller_native_javascript.cc
(right):

https://codereview.chromium.org/1265843005/diff/40001/components/dom_distille...
components/dom_distiller/content/renderer/distiller_native_javascript.cc:59:
render_frame_->GetServiceRegistry()->ConnectToRemoteService(
On 2015/09/11 17:14:23, nyquist wrote:
> On 2015/09/11 16:50:13, mdjones wrote:
> > On 2015/09/10 19:04:13, nyquist wrote:
> > > Is this a blocking call?
> > 
> > No, from my understanding you immediately get back a handle to a pipe which
is
> > either connected or isn't (allowing messages to be put into it either way).
> The
> > only blocking call I am aware of is the one to get a return value from a
call
> to
> > a remote service.
> 
> So if it's not blocking, what will happen when you later call
> distiller_js_service_->HandleFoobarCall(...)?

It fails quietly. It is possible to check for an error but in this case is
unhelpful.

commit-bot: I haz the power

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

5 years, 3 months ago (2015-09-24 15:23:55 UTC) #11

commit-bot: I haz the power

5 years, 3 months ago (2015-09-24 16:45:23 UTC) #13

Message was sent while issue was closed.

Patchset 5 (id:??) landed as
https://crrev.com/65173ea8d88884cf71b5713165c4c069b9b714dc
Cr-Commit-Position: refs/heads/master@{#350574}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages