Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 9 years ago

Currently, the dictionary lookup doesn't work on e10s. We should fix it.

	Tracy Walker [:tracy]
	Comment 1 • 9 years ago

Please provide steps to reproduce with expected results.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 2 • 9 years ago

Steps to reproduce:
1. open a page with e10s enabled
2. click the page to make sure it is focused (we should also get rid of this step in bug 569331)
3. use three finger tap on a word in the page

Expected result:
the dictionary popup appears just like e10s is not enabled

Actual result:
the popup does not appear

	Mike Conley (:mconley) (:⚙️)
	Comment 3 • 9 years ago

Hey Markus - will the gesture work in bug 863514 potentially fix this?

	Markus Stange [:mstange]
	Comment 4 • 9 years ago

Unfortunately not, no. Masayuki, are you working on this?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 5 • 9 years ago

I'm working on improving this feature without e10s. For supporting this in e10s, we need synchronous communication from chrome process to content process. This bug should fix that. But I'm not sure how to do it and who can allow to do that.

I think that we should use synchronous communication when it's impossible to cache, like NS_QUERY_CONTENT_CHAR_AT_POINT and NS_QUERY_CONTENT_TEXT_RECT. Note that Chromium does that for all cases at supporting this and IME, but we should try to avoid that as far as possible.

	Makoto Kato [:m_kato] Assignee
	Comment 6 • 9 years ago

Chromium uses synchronous with 1.5s timeout.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 7 • 9 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> I think that we should use synchronous communication when it's impossible to
> cache, like NS_QUERY_CONTENT_CHAR_AT_POINT and NS_QUERY_CONTENT_TEXT_RECT.
> Note that Chromium does that for all cases at supporting this and IME, but
> we should try to avoid that as far as possible.

I suggest we should avoid cache and use synchronous communication as far as possible if the requests are generally fast enough. Caching the whole text in documents could have a really bad memory footprint. Also, transfering that content between processes could also be expensive.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 8 • 9 years ago

> Also, transfering that content between processes could also be expensive.

Yep, it's expensive but "when" it's done is important, therefore, caching the content may improve the response speed. And also querying content is also expensive. So, for example, multiple querying text could be faster than non-e10s mode.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 10 • 9 years ago

According to the chromium developer, OS X has asynchronous-aware API:

https://code.google.com/p/chromium/issues/detail?id=121917#c32

> At least on 10.8, it's possible there's now enough API to do this async.
> 
> 10.8 adds - (void)quickLookWithEvent:(NSEvent *)event to NSResponder. RWHVMac could listen to that,
> and then call NSView's - (void)showDefinitionForAttributedString:(NSAttributedString *)attrString 
> atPoint:(NSPoint)textBaselineOrigin later when the results come back from the renderer (if the view
> hasn't scrolled in the meantime, possibly).

https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSResponder_Class/#//apple_ref/occ/instm/NSResponder/quickLookWithEvent:
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSView_Class/#//apple_ref/occ/instm/NSView/showDefinitionForAttributedString:atPoint:

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 11 • 9 years ago

http://mxr.mozilla.org/chromium/source/src/content/browser/renderer_host/render_widget_host_view_mac.mm#2973
> 2971 // This is invoked only on 10.8 or newer when the user taps a word using
> 2972 // three fingers.
> 2973 - (void)quickLookWithEvent:(NSEvent*)event {
> 2974   NSPoint point = [self convertPoint:[event locationInWindow] fromView:nil];
> 2975   TextInputClientMac::GetInstance()->GetStringAtPoint(
> 2976       renderWidgetHostView_->render_widget_host_,
> 2977       gfx::Point(point.x, NSHeight([self frame]) - point.y),
> 2978       ^(NSAttributedString* string, NSPoint baselinePoint) {
> 2979           if (string && [string length] > 0) {
> 2980             dispatch_async(dispatch_get_main_queue(), ^{
> 2981                 [self showDefinitionForAttributedString:string
> 2982                                                 atPoint:baselinePoint];
> 2983             });
> 2984           }
> 2985       }
> 2986   );
> 2987 }
> 2988

	ovidiu boca[:Ovidiu]
	Comment 12 • 8 years ago

Build ID 	20160205004003
User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:46.0) Gecko/20100101 Firefox/46.0


Tested on Mac OS X 10.10 with the Aurora 46.0a2 build. I can reproduce the problem.

	Erin Lancaster [:elan]
	Comment 13 • 8 years ago

Jeff says this blocks M9 and should be a P1. We can't regress basic functionality like this.

	Makoto Kato [:m_kato] Assignee
	Comment 14 • 8 years ago

I think that
- new WidgetCommandEvent ? for looking up dictionary
- Implement nsIWidget::LookupDictionary(in AString  selectedString, in long x, int long y) on PuppetWidget and nsChildView
- call showDefinitionForAttributedString in this method

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 15 • 8 years ago

Kato-san, thanks for taking this. I think we should fix this on e10s as well as OS X 10.11. Please see my notes in bug 1212527 comment 27 for some potential useful information.

	Makoto Kato [:m_kato] Assignee
	Comment 16 • 8 years ago

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #15)
> Kato-san, thanks for taking this. I think we should fix this on e10s as well
> as OS X 10.11. Please see my notes in bug 1212527 comment 27 for some
> potential useful information.

Thanks, Xidorn.  I will create a prototype at this week or next.

And if to support cmd+ctl+d key, we need new WidgetContentCommandEvent for this like copy&paste.

	Makoto Kato [:m_kato] Assignee
	Comment 17 • 8 years ago

WIP...

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 18 • 8 years ago

Comment on attachment 8728886 [details] [diff] [review]
WIP

Review of attachment 8728886 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/WidgetUtils.cpp
@@ +137,5 @@
> +    return false;
> +  }
> +
> +  nsCOMPtr<nsIWordBreaker> wordBreaker =
> +    do_GetService("@mozilla.org/intl/wbrk;1");

Our current implementation of WordBreaker won't work for languages like Chinese or Japanese. I guess it is probably fine for an initial version, but we would eventually want to use the framework Apple provides (see bug 1212527 comment 21), so that we have the identical behavior as native apps.

	Jim Mathies [:jimm]
	Comment 19 • 8 years ago

Makoto, is this really dependent on bug 569331 and bug 960480? Neither of those has seen any activity since last summer. (This bug currently blocks our initial rollout.)

	Makoto Kato [:m_kato] Assignee
	Comment 20 • 8 years ago

(In reply to Jim Mathies [:jimm] from comment #19)
> Makoto, is this really dependent on bug 569331 and bug 960480? Neither of
> those has seen any activity since last summer. (This bug currently blocks
> our initial rollout.)

No.  This bug is focused to implement 10.8 or later.

- From 10.8, it has new interface (quickLookWithEvent) to look up dictionary.  It is async-aware.
- Until 10.10, dictionary lookup doesn't work on e10s.  But it works on non-e10s.
- From 10.11, dictionary lookup doesn't work even if non-e10s.  10.11 doesn't support old API way.
- 10.6 and 10.7, it is no async API.  So I cannot find good way to support this on e10 yet.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 21 • 8 years ago

Jeff, see comment 20, this situation is more complicated than we thought when we originally triaged this. This doesn't work in non-e10s for 10.11 and there's no option to make this work for e10s in 10.7 and 10.6. So we're talking about a difference in behavior for users on 10.8, 10.9 and 10.10.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 22 • 8 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #21)
> Jeff, see comment 20, this situation is more complicated than we thought
> when we originally triaged this. This doesn't work in non-e10s for 10.11 and
> there's no option to make this work for e10s in 10.7 and 10.6. So we're
> talking about a difference in behavior for users on 10.8, 10.9 and 10.10.

10.6 -> 10.8 are being deprecated, let's not worry about them.

For 10.9 -> 10.11 I think this should be P1 but I'm not sure about blocking, given the breakage in non-e10s. 

Do we have a rough estimate of time to fix?

	Makoto Kato [:m_kato] Assignee
	Comment 23 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d49c9f4d712

	Makoto Kato [:m_kato] Assignee
	Comment 24 • 8 years ago

I would like to new method to look up dictionary.

Review commit: https://reviewboard.mozilla.org/r/43715/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43715/

	Makoto Kato [:m_kato] Assignee
	Comment 25 • 8 years ago

To get selected word on contnet process, I create new contnet command to look up dictionary.  Then, call nsIWidget's method to show looking up dictionary.

About fetched length, see https://www.chromium.org/developers/design-documents/system-dictionary-pop-up-architecture for OSX 10.6's design

Review commit: https://reviewboard.mozilla.org/r/43717/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43717/

	Makoto Kato [:m_kato] Assignee
	Comment 26 • 8 years ago

From 10.8+, it has new API to look up dictionary async.  So I use it.

Review commit: https://reviewboard.mozilla.org/r/43719/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43719/

	Makoto Kato [:m_kato] Assignee
	Comment 27 • 8 years ago

When using content command with params, it doesn't work well.
So I implement it for e10s.

Review commit: https://reviewboard.mozilla.org/r/43721/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43721/

	Makoto Kato [:m_kato] Assignee
	Comment 28 • 8 years ago

Implement LookUpDictionary to PuppetWidget and PBrowser for e10s.

Review commit: https://reviewboard.mozilla.org/r/43723/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43723/

	Makoto Kato [:m_kato] Assignee
	Comment 29 • 8 years ago

My fix that fetch length is ±50 at mouse point. According to https://www.chromium.org/developers/design-documents/system-dictionary-pop-up-architecture, string is -10 to +50, but rect is -50 to +50.  Should I change same value?

	Makoto Kato [:m_kato] Assignee
	Comment 30 • 8 years ago

I would like to new method to look up dictionary.

Review commit: https://reviewboard.mozilla.org/r/43731/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43731/

	Makoto Kato [:m_kato] Assignee
	Comment 31 • 8 years ago

To get selected word on contnet process, I create new contnet command to look up dictionary.  Then, call nsIWidget's method to show looking up dictionary.

About fetched length, see https://www.chromium.org/developers/design-documents/system-dictionary-pop-up-architecture for OSX 10.6's design

Review commit: https://reviewboard.mozilla.org/r/43733/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43733/

	Makoto Kato [:m_kato] Assignee
	Comment 32 • 8 years ago

From 10.8+, it has new API to look up dictionary async.  So I use it.

Review commit: https://reviewboard.mozilla.org/r/43735/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43735/

	Makoto Kato [:m_kato] Assignee
	Comment 33 • 8 years ago

When using content command with params, it doesn't work well.
So I implement it for e10s.

Review commit: https://reviewboard.mozilla.org/r/43737/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43737/

	Makoto Kato [:m_kato] Assignee
	Comment 34 • 8 years ago

Implement LookUpDictionary to PuppetWidget and PBrowser for e10s.

Review commit: https://reviewboard.mozilla.org/r/43739/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43739/

	Ricky Chien [:rickychien] (inactive)
	Comment 35 • 8 years ago

\o/ As a mac user, I'm waiting this for a long time. thank you m_kato :)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 36 • 8 years ago

Comment on attachment 8737084 [details]
MozReview Request: Bug 1177943 - Part 1. Add LookUpDictionary method to widget. r=masayuki

https://reviewboard.mozilla.org/r/43731/#review40393

> I would like to new method to look up dictionary.

I guess you meant "I would like to _add_ a new method to look up directionary".

::: widget/cocoa/nsChildView.mm:2860
(Diff revision 1)
>    }
>  }
>  
> +void
> +nsChildView::LookUpDictionary(
> +               const nsString& aText,

aText Should be nsAString.

::: widget/cocoa/nsChildView.mm:2873
(Diff revision 1)
> +  if (aIsVertical) {
> +    [attrStr addAttribute:NSVerticalGlyphFormAttributeName
> +                    value:[NSNumber numberWithInt: 1]
> +                    range:NSMakeRange(0, [attrStr length])];
> +  }

Hmm, looks like that this should be in nsCocoaUtils::GetNSMutableAttributedString() because I guess that other callers in the future may also need the writing mode too.

::: widget/cocoa/nsChildView.mm:2880
(Diff revision 1)
> +    if (aIsVertical) {
> +      pt.x -= [font descender];
> +    } else {
> +      pt.y += [font ascender];
> +    }

I'm not sure if this does right thing. IIRC, the (0, 0) of Mac is bottom-left of the screen. So, I guess that adding ascender means the aPoint.y is top of the text. And similarly, aPt.x is left of the text.

Anyway, you must have already tested these patches actually. So, I guess this works fine.

::: widget/nsBaseWidget.h:448
(Diff revision 1)
> +  virtual void LookUpDictionary(
> +                 const nsString& aText,
> +                 const nsTArray<mozilla::FontRange>& aFontRangeArray,
> +                 const bool aIsVertical,
> +                 const LayoutDeviceIntPoint& aPoint) override
> +               { }
> +

I think that this isn't necessary. See following issue.

::: widget/nsIWidget.h:2068
(Diff revision 1)
> +    virtual void LookUpDictionary(
> +                   const nsString& aText,
> +                   const nsTArray<mozilla::FontRange>& aFontRangeArray,
> +                   const bool aIsVertical,
> +                   const LayoutDeviceIntPoint& aPoint) = 0;
> +

I don't think that this should be an abstract method because you implement this in nsBaseWidget for non-Mac environments. So, this should be |{}| instead of |=0;|.

And could you add comment to explian this method and each argument?

::: widget/nsIWidget.h:2069
(Diff revision 1)
>                              const FrameMetrics::ViewID& aViewId,
>                              const CSSRect& aRect,
>                              const uint32_t& aFlags) = 0;
>  
> +    virtual void LookUpDictionary(
> +                   const nsString& aText,

aText should be nsAString rather than nsString.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 37 • 8 years ago

Comment on attachment 8737085 [details]
MozReview Request: Bug 1177943 - Part 2. Add cmd_lookUpDictionary content command. r=masayuki

https://reviewboard.mozilla.org/r/43733/#review40411

::: dom/base/nsGlobalWindowCommands.cpp:993
(Diff revision 1)
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSICONTROLLERCOMMAND
> +
> +private:
> +  ~nsLookUpDictionaryCommand()

Shouldn't be virtual because this is a subclass.

::: dom/base/nsGlobalWindowCommands.cpp:1019
(Diff revision 1)
> +nsLookUpDictionaryCommand::DoCommand(const char *aCommandName,
> +                                     nsISupports *aCommandContext)

nit: * should be after the type name instead of before the argument name.

::: dom/base/nsGlobalWindowCommands.cpp:1029
(Diff revision 1)
> +
> +NS_IMETHODIMP
> +nsLookUpDictionaryCommand::DoCommandParams(const char* aCommandName,
> +                                           nsICommandParams* aParams,
> +                                           nsISupports* aCommandContext)
> +{

I think that you should check |nsContentUtils::IsSafeToRunScript()| first because ContentEventHandler will flush pending layout. It may be dangureous if this command is sent by some event listeners.

I think that returning NS_ERROR_NOT_AVAILABLE or NS_ERROR_FAILURE is the right choice.

::: dom/base/nsGlobalWindowCommands.cpp:1050
(Diff revision 1)
> +  nsCOMPtr<nsPIDOMWindowOuter> window = do_QueryInterface(aCommandContext);
> +  if (NS_WARN_IF(!window)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsIDocShell *docShell = window->GetDocShell();

nit: nsIDocShell* docShell...

::: dom/base/nsGlobalWindowCommands.cpp:1076
(Diff revision 1)
> +  charAt.refPoint.x = x;
> +  charAt.refPoint.y = y;
> +  ContentEventHandler handler(presContext);
> +  handler.OnQueryCharacterAtPoint(&charAt);
> +
> +  if (!charAt.mSucceeded ||

NS_WARN_IF(!charAt.mSucceeded) ||...

::: dom/base/nsGlobalWindowCommands.cpp:1092
(Diff revision 1)
> +  } else {
> +    offset = 0;
> +  }
> +  textContent.InitForQueryTextContent(offset, 100);
> +  handler.OnQueryTextContent(&textContent);
> +  if (!textContent.mSucceeded || textContent.mReply.mString.IsEmpty()) {

NS_WARN_IF(!textContent.mSucceeded) ||...

::: dom/base/nsGlobalWindowCommands.cpp:1099
(Diff revision 1)
> +  }
> +
> +  // XXX nsIWordBreaker doesn't use contextual breaker.
> +  // If OS provides it, widget should use it if contextual breaker is needed.
> +  nsCOMPtr<nsIWordBreaker> wordBreaker = nsContentUtils::WordBreaker();
> +  if (!wordBreaker) {

NS_WARN_IF(!wordBreaker)?

::: dom/base/nsGlobalWindowCommands.cpp:1118
(Diff revision 1)
> +  WidgetQueryContentEvent lookUpContent(true, eQueryTextContent, widget);
> +  lookUpContent.InitForQueryTextContent(range.mBegin,
> +                                        range.mEnd - range.mBegin);
> +  lookUpContent.RequestFontRanges();
> +  handler.OnQueryTextContent(&lookUpContent);
> +  if (!lookUpContent.mSucceeded || lookUpContent.mReply.mString.IsEmpty()) {

NS_WARN_IF(!lookUpContent.mSucceeded) ||...

::: dom/base/nsGlobalWindowCommands.cpp:1125
(Diff revision 1)
> +  }
> +
> +  WidgetQueryContentEvent charRect(true, eQueryTextRect, widget);
> +  charRect.InitForQueryTextRect(range.mBegin, range.mEnd - range.mBegin);
> +  handler.OnQueryTextRect(&charRect);
> +  if (!charRect.mSucceeded) {

NS_WARN_IF(!charRect.mSucceeded)

::: widget/EventMessageList.h:302
(Diff revision 1)
>  NS_EVENT_MESSAGE(eContentCommandPaste)
>  NS_EVENT_MESSAGE(eContentCommandDelete)
>  NS_EVENT_MESSAGE(eContentCommandUndo)
>  NS_EVENT_MESSAGE(eContentCommandRedo)
>  NS_EVENT_MESSAGE(eContentCommandPasteTransferable)
> +NS_EVENT_MESSAGE(eContentCommandLookUpDictionary)

eContentCommandLookUpDictionaryAtPoint is better? Because this name doesn't indicate the refPoint is important.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 38 • 8 years ago

Comment on attachment 8737086 [details]
MozReview Request: Bug 1177943 - Part 3. Look up dictionary using new 10.8+ interface. r=masayuki

https://reviewboard.mozilla.org/r/43735/#review40419

::: widget/cocoa/nsChildView.mm:5408
(Diff revision 1)
> +  // Show dictionary by current point
> +  WidgetContentCommandEvent
> +    contentCommandEvent(true, eContentCommandLookUpDictionary, mGeckoChild);
> +  NSPoint point = [self convertPoint:[event locationInWindow] fromView:nil];
> +  contentCommandEvent.refPoint = mGeckoChild->CocoaPointsToDevPixels(point);
> +  mGeckoChild->DispatchWindowEvent(contentCommandEvent);

Could you add a comment after this line?

// The widget might have been destroyed.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 39 • 8 years ago

Comment on attachment 8737087 [details]
MozReview Request: Bug 1177943 - Part 4. Implement remote nsICommandWithParams. r=smaug

https://reviewboard.mozilla.org/r/43737/#review40421

I think that I shouldn't be a good person to review this patch. I'm not sure who is the best person for this, though...

::: docshell/base/nsDocShell.cpp:13363
(Diff revision 1)
> +  nsresult rv = GetControllerForCommand(aCommand, getter_AddRefs(controller));
> +  NS_ENSURE_SUCCESS(rv, rv);

You should use NS_WARN_IF() instead of NS_ENSURE_*().

::: docshell/base/nsDocShell.cpp:13368
(Diff revision 1)
> +  nsresult rv = GetControllerForCommand(aCommand, getter_AddRefs(controller));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsICommandController> commandController =
> +    do_QueryInterface(controller, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

Same here.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 40 • 8 years ago

Comment on attachment 8737088 [details]
MozReview Request: Bug 1177943 - Part 5. Add IPC for e10s support. r=masayuki

https://reviewboard.mozilla.org/r/43739/#review40423

::: dom/ipc/PBrowser.ipdl:441
(Diff revision 1)
> +     * @param aText       The word to look up
> +     * @param aFontRange  Text decoration of aText
> +     * @param aIsVertical true if vertical layout
> +     * @param aPoint      Top-left point of the word
> +     */
> +    async LookUpDictionary(nsString aText, FontRange[] aFontRanges,

I'm not sure if nsAString is available in *.ipdl. If it's available, please use it.  Otherwise, this is okay.

::: widget/PuppetWidget.h:266
(Diff revision 1)
>                            const uint32_t& aFlags) override;
>  
>    virtual bool HasPendingInputEvent() override;
>  
> +  virtual void LookUpDictionary(
> +                 const nsString& aText,

aText should be nsAString.

	Makoto Kato [:m_kato] Assignee
	Comment 41 • 8 years ago

https://reviewboard.mozilla.org/r/43731/#review40393

> I'm not sure if this does right thing. IIRC, the (0, 0) of Mac is bottom-left of the screen. So, I guess that adding ascender means the aPoint.y is top of the text. And similarly, aPt.x is left of the text.
> 
> Anyway, you must have already tested these patches actually. So, I guess this works fine.

showDefinitionForAttributedString requires baseline for atPoint.  It isn't top-left.

> aText should be nsAString rather than nsString.

OK.  But, when implementing IPC via PBrowser, PBrowser has to use nsString that nsAString is abstruct class.  So PuppetWidget will use nsString for IPC.

	Makoto Kato [:m_kato] Assignee
	Comment 42 • 8 years ago

https://reviewboard.mozilla.org/r/43739/#review40423

> I'm not sure if nsAString is available in *.ipdl. If it's available, please use it.  Otherwise, this is okay.

nsAString is abstract class, it cannot use it on IPDL.

	Makoto Kato [:m_kato] Assignee
	Comment 43 • 8 years ago

Comment on attachment 8737084 [details]
MozReview Request: Bug 1177943 - Part 1. Add LookUpDictionary method to widget. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43731/diff/1-2/

	Makoto Kato [:m_kato] Assignee
	Comment 44 • 8 years ago

Comment on attachment 8737085 [details]
MozReview Request: Bug 1177943 - Part 2. Add cmd_lookUpDictionary content command. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43733/diff/1-2/

	Makoto Kato [:m_kato] Assignee
	Comment 45 • 8 years ago

Comment on attachment 8737086 [details]
MozReview Request: Bug 1177943 - Part 3. Look up dictionary using new 10.8+ interface. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43735/diff/1-2/

	Makoto Kato [:m_kato] Assignee
	Comment 46 • 8 years ago

Comment on attachment 8737087 [details]
MozReview Request: Bug 1177943 - Part 4. Implement remote nsICommandWithParams. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43737/diff/1-2/

	Makoto Kato [:m_kato] Assignee
	Comment 47 • 8 years ago

Comment on attachment 8737088 [details]
MozReview Request: Bug 1177943 - Part 5. Add IPC for e10s support. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43739/diff/1-2/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 48 • 8 years ago

Comment on attachment 8737087 [details]
MozReview Request: Bug 1177943 - Part 4. Implement remote nsICommandWithParams. r=smaug

https://reviewboard.mozilla.org/r/43737/#review42761

::: toolkit/modules/RemoteController.jsm:65
(Diff revision 2)
> +      };
> +    } else {
> +      throw Cr.NS_ERROR_NOT_IMPLEMENTED;
> +    }
> +    this._browser.messageManager.sendAsyncMessage(
> +      "ControllerCommands:DoWithParams", JSON.stringify(cmd));

Why all this JSON handling here and on the receiving side? messageManager uses structured cloning internally and it should be able to handle this kind of stuff automatically

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 49 • 8 years ago

Comment on attachment 8737084 [details]
MozReview Request: Bug 1177943 - Part 1. Add LookUpDictionary method to widget. r=masayuki

https://reviewboard.mozilla.org/r/43731/#review42783

Could you fix the commit message as I said above?

> I would like to new method to look up dictionary.

I still guess that you meant "to _add_ new method".

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 50 • 8 years ago

Comment on attachment 8737085 [details]
MozReview Request: Bug 1177943 - Part 2. Add cmd_lookUpDictionary content command. r=masayuki

https://reviewboard.mozilla.org/r/43733/#review42789

With the fix, r+.

::: dom/base/nsGlobalWindowCommands.cpp:1082
(Diff revisions 1 - 2)
>    ContentEventHandler handler(presContext);
>    handler.OnQueryCharacterAtPoint(&charAt);
>  
> -  if (!charAt.mSucceeded ||
> -      charAt.mReply.mOffset == WidgetQueryContentEvent::NOT_FOUND) {
> +  if (NS_WARN_IF(
> +        !charAt.mSucceeded ||
> +        charAt.mReply.mOffset == WidgetQueryContentEvent::NOT_FOUND)) {

charAt.mReply.mOffset can be NOT_FOUND. So, this should be:

NS_WARN_IF(!charAt.mSucceeded) ||
charAt.mReply.mOffset == WidgetQueryContentEvent::NOT_FOUND

	Makoto Kato [:m_kato] Assignee
	Comment 51 • 8 years ago

Comment on attachment 8737084 [details]
MozReview Request: Bug 1177943 - Part 1. Add LookUpDictionary method to widget. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43731/diff/2-3/

	Makoto Kato [:m_kato] Assignee
	Comment 52 • 8 years ago

Comment on attachment 8737085 [details]
MozReview Request: Bug 1177943 - Part 2. Add cmd_lookUpDictionary content command. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43733/diff/2-3/

	Makoto Kato [:m_kato] Assignee
	Comment 53 • 8 years ago

Comment on attachment 8737086 [details]
MozReview Request: Bug 1177943 - Part 3. Look up dictionary using new 10.8+ interface. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43735/diff/2-3/

	Makoto Kato [:m_kato] Assignee
	Comment 54 • 8 years ago

Comment on attachment 8737087 [details]
MozReview Request: Bug 1177943 - Part 4. Implement remote nsICommandWithParams. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43737/diff/2-3/

	Makoto Kato [:m_kato] Assignee
	Comment 55 • 8 years ago

Comment on attachment 8737088 [details]
MozReview Request: Bug 1177943 - Part 5. Add IPC for e10s support. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43739/diff/2-3/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 56 • 8 years ago

Comment on attachment 8737087 [details]
MozReview Request: Bug 1177943 - Part 4. Implement remote nsICommandWithParams. r=smaug

https://reviewboard.mozilla.org/r/43737/#review49798

Given that this is dealing with coordinates I assume you've tested highdpi and non-highdpi, iframes, window maximized and not-maximized, etc.

	Makoto Kato [:m_kato] Assignee
	Comment 57 • 8 years ago

Ah, I will add additional fix to support HiDPI correctly...

	Makoto Kato [:m_kato] Assignee
	Comment 58 • 8 years ago

Part 4 isn't compatible with HiDPI mode.  getBoundingClientRect returns logical pixel, so we need convert to device pixel.

Review commit: https://reviewboard.mozilla.org/r/53488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53488/

	Makoto Kato [:m_kato] Assignee
	Comment 59 • 8 years ago

Comment on attachment 8737084 [details]
MozReview Request: Bug 1177943 - Part 1. Add LookUpDictionary method to widget. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43731/diff/3-4/

	Makoto Kato [:m_kato] Assignee
	Comment 60 • 8 years ago

Comment on attachment 8737085 [details]
MozReview Request: Bug 1177943 - Part 2. Add cmd_lookUpDictionary content command. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43733/diff/3-4/

	Makoto Kato [:m_kato] Assignee
	Comment 61 • 8 years ago

Comment on attachment 8737086 [details]
MozReview Request: Bug 1177943 - Part 3. Look up dictionary using new 10.8+ interface. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43735/diff/3-4/

	Makoto Kato [:m_kato] Assignee
	Comment 62 • 8 years ago

Comment on attachment 8737087 [details]
MozReview Request: Bug 1177943 - Part 4. Implement remote nsICommandWithParams. r=smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43737/diff/3-4/

	Makoto Kato [:m_kato] Assignee
	Comment 63 • 8 years ago

Comment on attachment 8737088 [details]
MozReview Request: Bug 1177943 - Part 5. Add IPC for e10s support. r=masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43739/diff/3-4/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 64 • 8 years ago

Comment on attachment 8753797 [details]
MozReview Request: Bug 1177943 - Part 6. Fix HiDPI support. r?smaug

https://reviewboard.mozilla.org/r/53488/#review50954

So please document somewhere what kind of coordinates aCommandParams is supposed to contain

	Pulsebot
	Comment 65 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d355aa36fa26
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b63f3496a31
https://hg.mozilla.org/integration/mozilla-inbound/rev/024599d425ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d08027b095
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4093f040496
https://hg.mozilla.org/integration/mozilla-inbound/rev/daec005079bf

	Carsten Book [:Tomcat]
	Comment 66 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d355aa36fa26
https://hg.mozilla.org/mozilla-central/rev/4b63f3496a31
https://hg.mozilla.org/mozilla-central/rev/024599d425ef
https://hg.mozilla.org/mozilla-central/rev/f9d08027b095
https://hg.mozilla.org/mozilla-central/rev/a4093f040496
https://hg.mozilla.org/mozilla-central/rev/daec005079bf

	Kris Maglione [:kmag]
	Comment 67 • 8 years ago

Part 4 of this is causing a NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO at [1] when we try to get a nsIWebNavigation.document property at [2].

I have no idea why this is happening, but changing the nsICommandParams parameter in doCommandWithParams to nsISupports in nsIDocShell.idl fixes it. This breaks the mochitests that depend on that file when they're run locally, but not when run test infra. I'm guessing it's because it only happens when those tests run early, but I still don't understand why.

[1]: https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp#1516
[2]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-backgroundPage.js#48

	Makoto Kato [:m_kato] Assignee
	Comment 68 • 8 years ago

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #18)

> Our current implementation of WordBreaker won't work for languages like
> Chinese or Japanese. I guess it is probably fine for an initial version, but
> we would eventually want to use the framework Apple provides (see bug
> 1212527 comment 21), so that we have the identical behavior as native apps.

filed as bug 1275486

	Kris Maglione [:kmag]
	Comment 69 • 8 years ago

Reopening until comment 67 and bug 1275719 are resolved. This may need to be backed out if we can't fix it before the merge.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 70 • 8 years ago

Please keep this fixed, and reopen once you back the patch out.

	steve
	Comment 71 • 8 years ago

Verified that this works. 49.0a1 (2016-05-31) + OS X 10.11.5.

	steve
	Comment 72 • 8 years ago

Any chance for this fix to be pushed to earlier versions of Firefox? By the time 49 becomes stable, we will have to deal with Firefox being broken again on OS X 10.12 likely.

	Makoto Kato [:m_kato] Assignee
	Comment 73 • 8 years ago

(In reply to steve-_- from comment #72)
> Any chance for this fix to be pushed to earlier versions of Firefox? By the
> time 49 becomes stable, we will have to deal with Firefox being broken again
> on OS X 10.12 likely.

To support this feature on 10.11, we need this fix and bug 1212527.  But since these fixes are large, we should not uplift this to aurora and beta.

	Jim Mathies [:jimm]
	Comment 74 • 8 years ago

too significant of a a change for 48 uplift.