Give NSAttributedStrings correct text styling attributes / Dictionary overlay has wrong font

RESOLVED FIXED in mozilla38

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: mstange, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
All
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 448503 [details]
Firefox screenshot: wrong font

When pressing Cmd+Ctrl+D in native OS X text fields or in Webkit, a dictionary panel pops up and the translated word is marked with a gray background and black text. This marker overlay gets the correct font style from an NSAttributedString that is returned by the queried NSView.

Bug 301451 will make dictionary look-up work in Mozilla. However, we don't provide any styling attributes in the returned NSAttributedString, so the marker overlay looks all wrong. Compare the screenshots in this bug.

The plain text we return is built in GenerateFlatTextContent in nsContentEventHandler.cpp which concatenates nsTextFragments returned by nsGenericDOMDataNode::GetText(). Unfortunately I don't see a simple way of getting the text style at that point...

Jonathan, where does our font rendering use the font style and where does it get it from? Do you see any way of pumping that data through to nsContentEventHandler.cpp?
(Reporter)

Comment 1

8 years ago
Created attachment 448504 [details]
Webkit screenshot: correct font

Comment 2

4 years ago
I am interested in working on this bug due to bug 301451. According to IRC, we somehow don't have access to nsStyleContext and I got the recommendation to check if jfkthame knows how to solve comment 0.
Flags: needinfo?(jfkthame)
You should be able to call nsRange::GetUsedFontFaces to find out what font(s) are being used for the range of text. (Though if this includes @font-face resources, they won't be meaningful to other applications.)

If the result contains more than one face, then you're looking at a range with mixed fonts; I suppose you could repeatedly bisect in order to find the boundaries between font runs, to set up an attributed string properly. This can't be done purely by walking the elements of the document, rather than examining its presentation, because font fallback (e.g. for different scripts) may mean we're using fonts that were not explicitly mentioned anywhere in the style data.

(More general background... We normally handle font style attributes during textframe reflow, when we need to create the frame's textrun in order to measure or draw glyphs. And to determine the actual font(s) used, we need not only the style-resolution work that layout does, but also the font-matching work from gfx. The style system gives us the value of the font-family property, but this is just a list of font family names; then gfx resolves these against the system's available fonts (and any @font-face rules in effect) to determine what's actually used.

See for example the code in nsTextFrame.cpp that calls nsLayoutUtils::GetFontMetricsForFrame to get an nsFontMetrics record, and then uses its gfxFontGroup to create a textrun. To find out what font(s) are actually used, you'd need to examine the glyph runs in the resulting gfxTextRun.)
Flags: needinfo?(jfkthame)
(Assignee)

Comment 4

4 years ago
Well, I don't think we have to handle such complex things mentioned above. It seems that, currently even Safari doesn't process the font fallback correctly. Hence, walking the elements and collecting font information from style data is enough, given the fact that we have no support of this feature yet.
(Assignee)

Updated

4 years ago
Assignee: nobody → quanxunzhen
(Assignee)

Comment 5

4 years ago
Created attachment 8517017 [details] [diff] [review]
WIP patch

I met some a problem with collapsed whitespaces. When a content has collapsed whitespaces, the font range won't sync with the content string. I currently don't have any idea about how to solve that.

jfkthame, do you have any advice about that? Please see the function AppendFontRanges, it uses GlyphRunIterator to get the ranges, however AppendString used in querying text content uses nsTextFragment. How can I sync the offset between them?
Flags: needinfo?(jfkthame)
That sounds like what gfxSkipChars is for, isn't it? By using ConvertSkippedToOriginal on the offsets from the glyph runs, you should get back the corresponding offsets in the content string. But from a quick look at your patch, that seems to be what you're doing already....so I'm not sure where there's still a problem. Am I looking at the wrong part of this?
Flags: needinfo?(jfkthame)
(Assignee)

Comment 7

4 years ago
Masayuki, I found there's a comment in IMEInputHandler::FirstRectForCharacterRange says:

  // XXX this returns first character rect or caret rect, it is limitation of
  // now. We need more work for returns first line rect. But current
  // implementation is enough for IMEs.

Only returning the rect for first character causes the highlight box positioned incorrectly when there are different fonts in a word. I changed the second parameter of InitForQueryTextRect from 1 to aRange.length, and it works.

Is the limitation still there now? Can I change that parameter and remove the comment?
Flags: needinfo?(masayuki)
(Assignee)

Comment 8

4 years ago
It seems that there is still the limitation, which causes the rect of the last line is used instead of the first line. We should file a followup bug for this. It is not a serious problem though.
Flags: needinfo?(masayuki)
(Assignee)

Comment 9

4 years ago
Created attachment 8518060 [details] [diff] [review]
patch
Attachment #8517017 - Attachment is obsolete: true
Attachment #8518060 - Flags: review?(masayuki)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #7)
> Masayuki, I found there's a comment in
> IMEInputHandler::FirstRectForCharacterRange says:
> 
>   // XXX this returns first character rect or caret rect, it is limitation of
>   // now. We need more work for returns first line rect. But current
>   // implementation is enough for IMEs.
> 
> Only returning the rect for first character causes the highlight box
> positioned incorrectly when there are different fonts in a word. I changed
> the second parameter of InitForQueryTextRect from 1 to aRange.length, and it
> works.
> 
> Is the limitation still there now?

Yes, there is. If the range is wrapped and split as two or more lines, the correct result is a rect which only includes all characters *only* in the first line. However, ContentEventHandler doesn't check if the range is wrapped. And there is no option in WidgetQueryContentEvent for retrieving only the first line's rect.

> Can I change that parameter and remove the comment?

If you can fix the issue mentioned above, you can remove the comment. I'll check your patch soon.
(Assignee)

Comment 11

4 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #7)
> > 
> > Is the limitation still there now?
> 
> Yes, there is. If the range is wrapped and split as two or more lines, the
> correct result is a rect which only includes all characters *only* in the
> first line. However, ContentEventHandler doesn't check if the range is
> wrapped. And there is no option in WidgetQueryContentEvent for retrieving
> only the first line's rect.

I see.

> > Can I change that parameter and remove the comment?
> 
> If you can fix the issue mentioned above, you can remove the comment. I'll
> check your patch soon.

No, I didn't. I'm going to file a new bug for that.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #8)
> It seems that there is still the limitation, which causes the rect of the
> last line is used instead of the first line. We should file a followup bug
> for this. It is not a serious problem though.

No, it's serious regression for Japanese IME. If composition string is wrapped, candidate window of IME may be positioned the left of second line. If the first line of composition string starts right-most of the line and the width of the editor is too wide like rich text editor, it causes providing much different UX.
(Assignee)

Comment 13

4 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #8)
> > It seems that there is still the limitation, which causes the rect of the
> > last line is used instead of the first line. We should file a followup bug
> > for this. It is not a serious problem though.
> 
> No, it's serious regression for Japanese IME. If composition string is
> wrapped, candidate window of IME may be positioned the left of second line.
> If the first line of composition string starts right-most of the line and
> the width of the editor is too wide like rich text editor, it causes
> providing much different UX.

My fault I didn't describe clearly. I meant, I realized that it would cause problem if I change it. And the current behavior (which causes the highlight box placed incorrect if there are different fonts in a word) is not a serious problem in this bug. Hence I won't touch it.
Okay, I see. I'll review your patch tonight or tomorrow morning if I can.
Comment on attachment 8518060 [details] [diff] [review]
patch

You are adding WidgetQueryContentEvent::mFlags but it conflicts with WidgetEvent::mFlags. So, you should not use the name.

On the other hand, you don't need to make the flags as an integer. You should just add a new bool member to WidgetQueryContentEvent. It's simpler and the event is never held for long time. So, you don't need to worry about the footprint. And you don't need to change WidgetQueryContentEvent. Perhaps, adding a method like |void WidgetQueryContentEvent::RequestFontRanges()| is enough.

Perhaps, |nsTArray<mozilla::FontRange> mFontRanges;| should be |nsAutoTArray<mozilla::FontRange, 10> mFontRanges;|? I'm not sure enough number of items in the most cases.

And please don't assume that nsTArray::size_type is size_t. You should use auto or define the type as WidgetQueryContentEvent::FontRangeArray and use FontRangeArray::size_type.

AppendFontRanges() should be reviewed by Jonathan Kew.

+static nsresult
+GenerateFlatFontRanges(nsRange* aRange, nsTArray<FontRange>& aFontRanges)
+{
+  NS_ASSERTION(aFontRanges.IsEmpty(), "aRanges must be empty array");

I guess that MOZ_ASSERT() is better.

+  nsINode* startNode = aRange->GetStartParent();
+  NS_ENSURE_TRUE(startNode, NS_ERROR_FAILURE);

Could you use NS_WARN_IF() instead of NS_ENSURE_TRUE()?

+  nsINode* endNode = aRange->GetEndParent();
+  NS_ENSURE_TRUE(endNode, NS_ERROR_FAILURE);

ditto.

+  // baseOffset is the flattened offset of each content node.
+  int32_t baseOffset = 0;
+  nsCOMPtr<nsIContentIterator> iter = NS_NewContentIterator();
+  for (iter->Init(aRange); !iter->IsDone(); iter->Next()) {
+    nsINode* node = iter->GetCurrentNode();
+    if (!node) {
+      break;
+    }
+    if (!node->IsNodeOfType(nsINode::eCONTENT)) {
+      continue;
+    }
+    nsIContent* content = static_cast<nsIContent*>(node);
+
+    if (content->IsNodeOfType(nsINode::eTEXT)) {
+      int32_t startOffset = content != startNode ? 0 : aRange->StartOffset();
+      int32_t endOffset = content != endNode ?
+        content->TextLength() : aRange->EndOffset();
+      nsresult rv = AppendFontRanges(aFontRanges, content,
+                                     baseOffset, startOffset, endOffset);
+      NS_ENSURE_SUCCESS(rv, rv);
+      baseOffset += endOffset - startOffset;

Note that the text may include some line breaks. So, please use ContentEventHandler::GetNativeTextLength().

+    } else if (IsContentBR(content)) {
+      // FIXME In Windows, it is wrong. But currently it's fine because
+      //       this method is used in OS X only.
+      baseOffset++;

Even if it's not used by Windows for now, please don't write such code in XP level. Other developers working on Windows probably won't check the implementation when they use this.

You can use ContentEventHandler::GetNativeTextLength() for this.
Attachment #8518060 - Flags: review?(masayuki) → review-
(Assignee)

Comment 16

4 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> Comment on attachment 8518060 [details] [diff] [review]
> patch
> 
> Note that the text may include some line breaks. So, please use
> ContentEventHandler::GetNativeTextLength().

If that happens, the ranges from AppendFontRanges will be wrong as well.

> +    } else if (IsContentBR(content)) {
> +      // FIXME In Windows, it is wrong. But currently it's fine because
> +      //       this method is used in OS X only.
> +      baseOffset++;
> 
> Even if it's not used by Windows for now, please don't write such code in XP
> level. Other developers working on Windows probably won't check the
> implementation when they use this.
> 
> You can use ContentEventHandler::GetNativeTextLength() for this.

I feel supporting Windows here will unnecessarily increase the complexity for this code. And this problem is not related to this bug at all. But I agree it is likely that developers won't check the impl before they use.
Hence I suggest that we add MOZ_ASSERT_UNREACHABLE on Windows, so that when developers really want it to work on Windows, they can be aware of this limitation. Do you think it is acceptable?
Flags: needinfo?(masayuki)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #16)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> > Comment on attachment 8518060 [details] [diff] [review]
> > patch
> > 
> > Note that the text may include some line breaks. So, please use
> > ContentEventHandler::GetNativeTextLength().
> 
> If that happens, the ranges from AppendFontRanges will be wrong as well.

I think you need to pass both XP offsets and native offsets. If the query event needs native offset, the font ranges should have offsets with native line breaks.

> > +    } else if (IsContentBR(content)) {
> > +      // FIXME In Windows, it is wrong. But currently it's fine because
> > +      //       this method is used in OS X only.
> > +      baseOffset++;
> > 
> > Even if it's not used by Windows for now, please don't write such code in XP
> > level. Other developers working on Windows probably won't check the
> > implementation when they use this.
> > 
> > You can use ContentEventHandler::GetNativeTextLength() for this.
> 
> I feel supporting Windows here will unnecessarily increase the complexity
> for this code. And this problem is not related to this bug at all. But I
> agree it is likely that developers won't check the impl before they use.
> Hence I suggest that we add MOZ_ASSERT_UNREACHABLE on Windows, so that when
> developers really want it to work on Windows, they can be aware of this
> limitation. Do you think it is acceptable?

No, it's not hard to implement. We should do it in this bug. Note that TSF (Windows' new IME API framework) may need this feature.
Flags: needinfo?(masayuki)
(Assignee)

Comment 18

4 years ago
Created attachment 8519717 [details] [diff] [review]
patch

Support native line break now, but I haven't test it under Windows.

In addition, I feel that it would have a terrible performance on Windows because of the current implementation of GetNativeTextLength. The time complexity may become O(N^2) in some cases. Anyway, I think we need to optimize that before we really start using it on Windows.
Attachment #8518060 - Attachment is obsolete: true
Attachment #8519717 - Flags: review?(masayuki)
Comment on attachment 8519717 [details] [diff] [review]
patch

>@@ -418,16 +423,129 @@ static nsresult GenerateFlatTextContent(
>     }
>   }
>   if (aLineBreakType == LINE_BREAK_TYPE_NATIVE) {
>     ConvertToNativeNewlines(aString);
>   }
>   return NS_OK;
> }
> 
>+static nsresult
>+AppendFontRanges(nsTArray<FontRange>& aFontRanges,
>+                 nsIContent* aContent, int32_t aBaseOffset,
>+                 int32_t aStartOffset, int32_t aEndOffset,

aXPStartOffset and aXPEndOffset? Looks like that they are always XP offsets.

>+                 LineBreakType aLineBreakType)
>+{
>+  auto curr = static_cast<nsTextFrame*>(aContent->GetPrimaryFrame());

nit: Perhaps, you should add MOZ_ASSERT() for checking if the frame is actually a text frame.

>+  while (curr) {
>+    int32_t fstart = std::max(curr->GetContentOffset(), aStartOffset);
>+    int32_t fend = std::min(curr->GetContentEnd(), aEndOffset);

nit: Please rename them. "f" isn't clear at reading below.

>+    if (fstart >= fend) {
>+      curr = static_cast<nsTextFrame*>(curr->GetNextContinuation());
>+      continue;
>+    }
>+
>+    gfxSkipCharsIterator iter = curr->EnsureTextRun(nsTextFrame::eInflated);
>+    gfxTextRun* textRun = curr->GetTextRun(nsTextFrame::eInflated);
>+    NS_ENSURE_TRUE(textRun, NS_ERROR_OUT_OF_MEMORY);

Please use NS_WARN_IF() instead of NS_ENSURE_TRUE. I wonder, does this actually occur? I *guess* that if OOM has already occurred, it's must have been crashed.

>+    nsTextFrame* next = nullptr;
>+    if (fend < aEndOffset) {
>+      next = static_cast<nsTextFrame*>(curr->GetNextContinuation());
>+      while (next && next->GetTextRun(nsTextFrame::eInflated) == textRun) {
>+        fend = std::min(next->GetContentEnd(), aEndOffset);
>+        next = fend < aEndOffset ?
>+          static_cast<nsTextFrame*>(next->GetNextContinuation()) : nullptr;
>+      }
>+    }
>+
>+    uint32_t skipStart = iter.ConvertOriginalToSkipped(fstart);
>+    uint32_t skipEnd = iter.ConvertOriginalToSkipped(fend);
>+    gfxTextRun::GlyphRunIterator runIter(
>+        textRun, skipStart, skipEnd - skipStart);
>+    while (runIter.NextRun()) {
>+      gfxFont* font = runIter.GetGlyphRun()->mFont.get();
>+      int32_t startOffset =
>+        iter.ConvertSkippedToOriginal(runIter.GetStringStart());
>+      // It is possible that the first glyph run has exceeded the frame,
>+      // because the whole frame is filled by skipped chars.
>+      if (startOffset >= fend) {
>+        break;
>+      }
>+      // The converted original offset may exceed the range,
>+      // hence we need to clamp it.
>+      int32_t endOffset =
>+        iter.ConvertSkippedToOriginal(runIter.GetStringEnd());
>+      endOffset = std::min(fend, endOffset);
>+
>+      FontRange* fontRange = aFontRanges.AppendElement();
>+      if (aLineBreakType == LINE_BREAK_TYPE_NATIVE) {
>+        fontRange->mOffset = aBaseOffset + ContentEventHandler::
>+          GetNativeTextLength(aContent, aStartOffset, startOffset);

nit: Why don't you make this method a static member of ContentEventHandler?

>+        fontRange->mLength = ContentEventHandler::
>+          GetNativeTextLength(aContent, startOffset, endOffset);

Why don't you cache the next offset here?

>+static nsresult
>+GenerateFlatFontRanges(nsRange* aRange, nsTArray<FontRange>& aFontRanges,
>+                       LineBreakType aLineBreakType)
>+{
>+  NS_ASSERTION(aFontRanges.IsEmpty(), "aRanges must be empty array");
>+
>+  nsINode* startNode = aRange->GetStartParent();
>+  nsINode* endNode = aRange->GetEndParent();
>+  NS_WARN_IF(!startNode || !endNode);

Don't you need to return here??

I.e.,

if (NS_WARN_IF(!startNode || !endNode)) {
  return NS_*;
}

>+
>+  // baseOffset is the flattened offset of each content node.
>+  int32_t baseOffset = 0;
>+  nsCOMPtr<nsIContentIterator> iter = NS_NewContentIterator();
>+  for (iter->Init(aRange); !iter->IsDone(); iter->Next()) {
>+    nsINode* node = iter->GetCurrentNode();
>+    if (!node) {
>+      break;
>+    }
>+    if (!node->IsNodeOfType(nsINode::eCONTENT)) {
>+      continue;
>+    }
>+    nsIContent* content = static_cast<nsIContent*>(node);
>+
>+    if (content->IsNodeOfType(nsINode::eTEXT)) {
>+      int32_t startOffset = content != startNode ? 0 : aRange->StartOffset();
>+      int32_t endOffset = content != endNode ?
>+        content->TextLength() : aRange->EndOffset();
>+      nsresult rv = AppendFontRanges(aFontRanges, content,
>+                                     baseOffset, startOffset, endOffset,
>+                                     aLineBreakType);
>+      NS_ENSURE_SUCCESS(rv, rv);

Please use NS_WARN_IF().

>+      if (aLineBreakType == LINE_BREAK_TYPE_NATIVE) {
>+        baseOffset += ContentEventHandler::
>+          GetNativeTextLength(content, startOffset, endOffset);

Please make this a static member of ContentEventHandler. Then, you can remove |ContentEventHandler::|.

And also, why don't you use the last font range instead of computing native text length again?

>+      } else {
>+        baseOffset += endOffset - startOffset;
>+      }
>+    } else if (IsContentBR(content)) {
>+      baseOffset += ContentEventHandler::GetBRLength(aLineBreakType);

Hmm, this causes that line breaks which are generated by <br> elements not being included in any ranges. Doesn't this make widget developers confused? I think that this method should guarantee that all font ranges start next offset of its previous range and never overlap each others.

>diff --git a/widget/FontRange.h b/widget/FontRange.h
>new file mode 100644
>--- /dev/null
>+++ b/widget/FontRange.h
>@@ -0,0 +1,31 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_FontRange_h_
>+#define mozilla_FontRange_h_
>+
>+namespace mozilla {
>+
>+struct FontRange
>+{
>+  FontRange()
>+    : mOffset(0)
>+    , mLength(0)
>+    , mFontSize(0)
>+    , mIsBold(false)
>+  {
>+  }
>+
>+  int32_t mOffset;
>+  int32_t mLength;
>+  nsString mFontName;

Perhaps, nsAutoString is better.

>+  gfxFloat mFontSize;

Please explain with comment this value is in pixels. (right?)

>+    // used by NS_QUERY_TEXT_CONTENT with font ranges requested
>+    nsTArray<mozilla::FontRange> mFontRanges;

Please use nsAutoTArray for preventing fragmentation.

>+    size_t fontRangeLength = aParam.mReply.mFontRanges.Length();
>+    WriteParam(aMsg, fontRangeLength);
>+    for (size_t i = 0; i < fontRangeLength; i++) {
>+      WriteParam(aMsg, aParam.mReply.mFontRanges[i]);
>+    }

Cannot you use |WriteParam(aMsg, aParam.mReply.mFontRanges)| simply?

>   }
> 
>   static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>   {
>     aResult->mWasAsync = true;
>-    return ReadParam(aMsg, aIter,
>-                     static_cast<mozilla::WidgetGUIEvent*>(aResult)) &&
>-           ReadParam(aMsg, aIter, &aResult->mSucceeded) &&
>-           ReadParam(aMsg, aIter, &aResult->mUseNativeLineBreak) &&
>-           ReadParam(aMsg, aIter, &aResult->mInput.mOffset) &&
>-           ReadParam(aMsg, aIter, &aResult->mInput.mLength) &&
>-           ReadParam(aMsg, aIter, &aResult->mReply.mOffset) &&
>-           ReadParam(aMsg, aIter, &aResult->mReply.mString) &&
>-           ReadParam(aMsg, aIter, &aResult->mReply.mRect) &&
>-           ReadParam(aMsg, aIter, &aResult->mReply.mReversed) &&
>-           ReadParam(aMsg, aIter, &aResult->mReply.mHasSelection) &&
>-           ReadParam(aMsg, aIter, &aResult->mReply.mWidgetIsHit);
>+    if (!ReadParam(aMsg, aIter,
>+                   static_cast<mozilla::WidgetGUIEvent*>(aResult)) ||
>+        !ReadParam(aMsg, aIter, &aResult->mSucceeded) ||
>+        !ReadParam(aMsg, aIter, &aResult->mUseNativeLineBreak) ||
>+        !ReadParam(aMsg, aIter, &aResult->mWithFontRanges) ||
>+        !ReadParam(aMsg, aIter, &aResult->mInput.mOffset) ||
>+        !ReadParam(aMsg, aIter, &aResult->mInput.mLength) ||
>+        !ReadParam(aMsg, aIter, &aResult->mReply.mOffset) ||
>+        !ReadParam(aMsg, aIter, &aResult->mReply.mString) ||
>+        !ReadParam(aMsg, aIter, &aResult->mReply.mRect) ||
>+        !ReadParam(aMsg, aIter, &aResult->mReply.mReversed) ||
>+        !ReadParam(aMsg, aIter, &aResult->mReply.mHasSelection) ||
>+        !ReadParam(aMsg, aIter, &aResult->mReply.mWidgetIsHit)) {
>+      return false;
>+    }
>+
>+    size_t fontRangeLength;
>+    if (!ReadParam(aMsg, aIter, &fontRangeLength)) {
>+      return false;
>+    }
>+    auto& fontRanges = aResult->mReply.mFontRanges;
>+    fontRanges.SetCapacity(fontRangeLength);
>+    for (size_t i = 0; i < fontRangeLength; i++) {
>+      auto* fontRange = fontRanges.AppendElement();
>+      if (!ReadParam(aMsg, aIter, fontRange)) {

ditto.


Sorry, I should've pointed some of them at previous review.
Attachment #8519717 - Flags: review?(masayuki) → review-
(Assignee)

Comment 20

4 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> Comment on attachment 8519717 [details] [diff] [review]
> patch
> 
> 
> >+        fontRange->mLength = ContentEventHandler::
> >+          GetNativeTextLength(aContent, startOffset, endOffset);
> 
> Why don't you cache the next offset here?

I'm not sure if the GlyphRun really guarantees that the ranges are continuous.

> 
> >+      if (aLineBreakType == LINE_BREAK_TYPE_NATIVE) {
> >+        baseOffset += ContentEventHandler::
> >+          GetNativeTextLength(content, startOffset, endOffset);
> 
> Please make this a static member of ContentEventHandler. Then, you can
> remove |ContentEventHandler::|.
> 
> And also, why don't you use the last font range instead of computing native
> text length again?

ditto. I'm not sure if it is guaranteed that the end offset of the last font range is truly the start offset of the next.

> >+      } else {
> >+        baseOffset += endOffset - startOffset;
> >+      }
> >+    } else if (IsContentBR(content)) {
> >+      baseOffset += ContentEventHandler::GetBRLength(aLineBreakType);
> 
> Hmm, this causes that line breaks which are generated by <br> elements not
> being included in any ranges. Doesn't this make widget developers confused?
> I think that this method should guarantee that all font ranges start next
> offset of its previous range and never overlap each others.

I don't think it is necessary to guarantee that. Each range has its own offset and length, which could indicate that it is not guaranteed to be continuous and independent. 

> >+  gfxFloat mFontSize;
> 
> Please explain with comment this value is in pixels. (right?)

If the document for gfxFontStyle is correct, it is in pixels. But Apple's document says it's API accepts value in points while simply passing it works fine. So I don't know. Maybe we should just follow our own documents.

> >+    size_t fontRangeLength = aParam.mReply.mFontRanges.Length();
> >+    WriteParam(aMsg, fontRangeLength);
> >+    for (size_t i = 0; i < fontRangeLength; i++) {
> >+      WriteParam(aMsg, aParam.mReply.mFontRanges[i]);
> >+    }
> 
> Cannot you use |WriteParam(aMsg, aParam.mReply.mFontRanges)| simply?

Do we have ParamTraits<nsTArray<T>>? I don't feel we should implement a ParamTraits for each type of array.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #20)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> > Comment on attachment 8519717 [details] [diff] [review]
> > patch
> > 
> > 
> > >+        fontRange->mLength = ContentEventHandler::
> > >+          GetNativeTextLength(aContent, startOffset, endOffset);
> > 
> > Why don't you cache the next offset here?
> 
> I'm not sure if the GlyphRun really guarantees that the ranges are
> continuous.

Okay, then, we should ask roc. Roc, let me us about this issue.

> > >+      if (aLineBreakType == LINE_BREAK_TYPE_NATIVE) {
> > >+        baseOffset += ContentEventHandler::
> > >+          GetNativeTextLength(content, startOffset, endOffset);
> > 
> > Please make this a static member of ContentEventHandler. Then, you can
> > remove |ContentEventHandler::|.
> > 
> > And also, why don't you use the last font range instead of computing native
> > text length again?
> 
> ditto. I'm not sure if it is guaranteed that the end offset of the last font
> range is truly the start offset of the next.

ditto.
Flags: needinfo?(roc)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #20)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> > >+      } else {
> > >+        baseOffset += endOffset - startOffset;
> > >+      }
> > >+    } else if (IsContentBR(content)) {
> > >+      baseOffset += ContentEventHandler::GetBRLength(aLineBreakType);
> > 
> > Hmm, this causes that line breaks which are generated by <br> elements not
> > being included in any ranges. Doesn't this make widget developers confused?
> > I think that this method should guarantee that all font ranges start next
> > offset of its previous range and never overlap each others.
> 
> I don't think it is necessary to guarantee that. Each range has its own
> offset and length, which could indicate that it is not guaranteed to be
> continuous and independent. 

Sounds strange. If some ranges overlap, which information should used by widget?

> > >+    size_t fontRangeLength = aParam.mReply.mFontRanges.Length();
> > >+    WriteParam(aMsg, fontRangeLength);
> > >+    for (size_t i = 0; i < fontRangeLength; i++) {
> > >+      WriteParam(aMsg, aParam.mReply.mFontRanges[i]);
> > >+    }
> > 
> > Cannot you use |WriteParam(aMsg, aParam.mReply.mFontRanges)| simply?
> 
> Do we have ParamTraits<nsTArray<T>>? I don't feel we should implement a
> ParamTraits for each type of array.

Yes:
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#465
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#570
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #20)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> > > >+      } else {
> > > >+        baseOffset += endOffset - startOffset;
> > > >+      }
> > > >+    } else if (IsContentBR(content)) {
> > > >+      baseOffset += ContentEventHandler::GetBRLength(aLineBreakType);
> > > 
> > > Hmm, this causes that line breaks which are generated by <br> elements not
> > > being included in any ranges. Doesn't this make widget developers confused?
> > > I think that this method should guarantee that all font ranges start next
> > > offset of its previous range and never overlap each others.
> > 
> > I don't think it is necessary to guarantee that. Each range has its own
> > offset and length, which could indicate that it is not guaranteed to be
> > continuous and independent. 
> 
> Sounds strange. If some ranges overlap, which information should used by
> widget?

And if there is not range which includes an offset. Then, what will do will depend on widget. That's too bad. E.g., inserting <br> to an empty block causes the block has one line-height. So, <br>'s information may be necessary.
(Assignee)

Comment 24

4 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #20)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> > > >+      } else {
> > > >+        baseOffset += endOffset - startOffset;
> > > >+      }
> > > >+    } else if (IsContentBR(content)) {
> > > >+      baseOffset += ContentEventHandler::GetBRLength(aLineBreakType);
> > > 
> > > Hmm, this causes that line breaks which are generated by <br> elements not
> > > being included in any ranges. Doesn't this make widget developers confused?
> > > I think that this method should guarantee that all font ranges start next
> > > offset of its previous range and never overlap each others.
> > 
> > I don't think it is necessary to guarantee that. Each range has its own
> > offset and length, which could indicate that it is not guaranteed to be
> > continuous and independent. 
> 
> Sounds strange. If some ranges overlap, which information should used by
> widget?

Anyway, I think my current code can at least guarantee that they don't overlap.

> > > >+    size_t fontRangeLength = aParam.mReply.mFontRanges.Length();
> > > >+    WriteParam(aMsg, fontRangeLength);
> > > >+    for (size_t i = 0; i < fontRangeLength; i++) {
> > > >+      WriteParam(aMsg, aParam.mReply.mFontRanges[i]);
> > > >+    }
> > > 
> > > Cannot you use |WriteParam(aMsg, aParam.mReply.mFontRanges)| simply?
> > 
> > Do we have ParamTraits<nsTArray<T>>? I don't feel we should implement a
> > ParamTraits for each type of array.
> 
> Yes:
> http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#465
> http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#570

OK, so I think we can simplify code in some places like:
http://dxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h#245
http://dxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h#445
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #24)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> > (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #20)
> > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> > > > >+      } else {
> > > > >+        baseOffset += endOffset - startOffset;
> > > > >+      }
> > > > >+    } else if (IsContentBR(content)) {
> > > > >+      baseOffset += ContentEventHandler::GetBRLength(aLineBreakType);
> > > > 
> > > > Hmm, this causes that line breaks which are generated by <br> elements not
> > > > being included in any ranges. Doesn't this make widget developers confused?
> > > > I think that this method should guarantee that all font ranges start next
> > > > offset of its previous range and never overlap each others.
> > > 
> > > I don't think it is necessary to guarantee that. Each range has its own
> > > offset and length, which could indicate that it is not guaranteed to be
> > > continuous and independent. 
> > 
> > Sounds strange. If some ranges overlap, which information should used by
> > widget?
> 
> Anyway, I think my current code can at least guarantee that they don't
> overlap.

Then, it might be better to insert new range or expand previous range for <br> or something causing noncontinuous.

> > > > >+    size_t fontRangeLength = aParam.mReply.mFontRanges.Length();
> > > > >+    WriteParam(aMsg, fontRangeLength);
> > > > >+    for (size_t i = 0; i < fontRangeLength; i++) {
> > > > >+      WriteParam(aMsg, aParam.mReply.mFontRanges[i]);
> > > > >+    }
> > > > 
> > > > Cannot you use |WriteParam(aMsg, aParam.mReply.mFontRanges)| simply?
> > > 
> > > Do we have ParamTraits<nsTArray<T>>? I don't feel we should implement a
> > > ParamTraits for each type of array.
> > 
> > Yes:
> > http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#465
> > http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#570
> 
> OK, so I think we can simplify code in some places like:
> http://dxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h#245
> http://dxr.mozilla.org/mozilla-central/source/widget/nsGUIEventIPC.h#445

Their element is refcountable class pointer. Therefore, I'm not sure if they can be replaced.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #20)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> > > Comment on attachment 8519717 [details] [diff] [review]
> > > patch
> > > 
> > > 
> > > >+        fontRange->mLength = ContentEventHandler::
> > > >+          GetNativeTextLength(aContent, startOffset, endOffset);
> > > 
> > > Why don't you cache the next offset here?
> > 
> > I'm not sure if the GlyphRun really guarantees that the ranges are
> > continuous.
> 
> Okay, then, we should ask roc. Roc, let me us about this issue.

Answering for roc... in the textrun, it's guaranteed that the ranges of the GlyphRuns are continuous. Each GlyphRun stores only a start offset; its length is implied by the start offset of the next GlyphRun or the end of the text, so there cannot be any gaps or overlaps.

(See http://hg.mozilla.org/mozilla-central/annotate/cbe6afcae26c/gfx/thebes/gfxTextRun.h#l413)

> > > >+      if (aLineBreakType == LINE_BREAK_TYPE_NATIVE) {
> > > >+        baseOffset += ContentEventHandler::
> > > >+          GetNativeTextLength(content, startOffset, endOffset);
> > > 
> > > Please make this a static member of ContentEventHandler. Then, you can
> > > remove |ContentEventHandler::|.
> > > 
> > > And also, why don't you use the last font range instead of computing native
> > > text length again?
> > 
> > ditto. I'm not sure if it is guaranteed that the end offset of the last font
> > range is truly the start offset of the next.
> 
> ditto.

Ditto. :)
Flags: needinfo?(roc)
(Assignee)

Comment 27

4 years ago
Created attachment 8521910 [details] [diff] [review]
patch
Attachment #8519717 - Attachment is obsolete: true
Attachment #8521910 - Flags: review?(masayuki)
Comment on attachment 8521910 [details] [diff] [review]
patch

>+    uint32_t skipStart = iter.ConvertOriginalToSkipped(frameXPStart);
>+    uint32_t skipEnd = iter.ConvertOriginalToSkipped(frameXPEnd);
>+    gfxTextRun::GlyphRunIterator runIter(
>+        textRun, skipStart, skipEnd - skipStart);

nit: indent should be 2 spaces.

>+    int32_t lastXPEndOffset = frameXPStart;
>+    while (runIter.NextRun()) {
>+      gfxFont* font = runIter.GetGlyphRun()->mFont.get();
>+      int32_t startXPOffset =
>+        iter.ConvertSkippedToOriginal(runIter.GetStringStart());
>+      // It is possible that the first glyph run has exceeded the frame,
>+      // because the whole frame is filled by skipped chars.
>+      if (startXPOffset >= frameXPEnd) {
>+        break;
>+      }
>+
>+      if (startXPOffset > lastXPEndOffset) {
>+        // Create range for skipped leading chars.
>+        baseOffset += AppendEmptyFontRange(aFontRanges, aContent, baseOffset,
>+                                           lastXPEndOffset, startXPOffset,
>+                                           aLineBreakType);

Perhaps, shouldn't you expand the previous range rather than appending empty font range? Or expand the range of next range? If so, widget doesn't meat "odd" font range. (but I'm not sure)

>+    if (lastXPEndOffset < frameXPEnd) {
>+      // Create range for skipped trailing chars. It also handles case
>+      // that the whole frame contains only skipped chars.
>+      baseOffset += AppendEmptyFontRange(aFontRanges, aContent, baseOffset,
>+                                         lastXPEndOffset, frameXPEnd,
>+                                         aLineBreakType);

ditto.

>+/* static */ nsresult
>+ContentEventHandler::GenerateFlatFontRanges(nsRange* aRange,
>+                                            nsTArray<FontRange>& aFontRanges,
>+                                            int32_t& aLength,
>+                                            LineBreakType aLineBreakType)
>+{
>+  MOZ_ASSERT(aFontRanges.IsEmpty(), "aRanges must be empty array");
>+
>+  nsINode* startNode = aRange->GetStartParent();
>+  nsINode* endNode = aRange->GetEndParent();
>+  if (NS_WARN_IF(!startNode || !endNode)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  // baseOffset is the flattened offset of each content node.
>+  int32_t baseOffset = 0;
>+  nsCOMPtr<nsIContentIterator> iter = NS_NewContentIterator();
>+  for (iter->Init(aRange); !iter->IsDone(); iter->Next()) {
>+    nsINode* node = iter->GetCurrentNode();
>+    if (!node) {

NS_WARN_IF()?

>+      break;
>+    }
>+    if (!node->IsNodeOfType(nsINode::eCONTENT)) {
>+      continue;
>+    }
>+    nsIContent* content = static_cast<nsIContent*>(node);

nit: You can use node->AsContent().

>+    if (content->IsNodeOfType(nsINode::eTEXT)) {
>+      int32_t startOffset = content != startNode ? 0 : aRange->StartOffset();
>+      int32_t endOffset = content != endNode ?
>+        content->TextLength() : aRange->EndOffset();
>+      baseOffset += AppendFontRanges(aFontRanges, content,
>+                                     baseOffset, startOffset, endOffset,
>+                                     aLineBreakType);
>+    } else if (IsContentBR(content)) {
>+      if (aFontRanges.IsEmpty()) {
>+        MOZ_ASSERT(baseOffset == 0);
>+        FontRange* fontRange = aFontRanges.AppendElement();
>+        nsIFrame* brFrame = content->GetPrimaryFrame();
>+        nscoord fontSize = brFrame->StyleFont()->mFont.size;
>+        fontRange->mFontSize = NSAppUnitsToIntPixels(fontSize,
>+                                                     AppUnitsPerCSSPixel());

How about fontface and bold information? In most cases, authors specify font styles to its ancestor block element. So, it may be useful.

>+  if (aEvent->mWithFontRanges) {
>+    int32_t fontRangeLength;
>+    rv = GenerateFlatFontRanges(range, aEvent->mReply.mFontRanges,
>+                                fontRangeLength, lineBreakType);
>+    NS_ENSURE_SUCCESS(rv, rv);

nit:

if (NS_WARNI_F(NS_FAILED(rv))) {
  return rv;
}

>+  // Get the length of a BR in the given line break type
>+  static uint32_t GetBRLength(LineBreakType aLineBreakType);

Making this |static inline| better?

>+struct FontRange
>+{
>+  FontRange()
>+    : mStartOffset(0)
>+    , mFontSize(0)
>+  {
>+  }
>+
>+  int32_t mStartOffset;
>+  nsAutoString mFontName;
>+  gfxFloat mFontSize; // in pixels
>+};

Wow, mIsBold isn't necessary? And also how about aIsItalic?

And I'm not sure if it's possible, though. If you can use nsStyleFont in WidgetEventImpl.cpp, you should create FontRange::SetStyle(const nsStyleFont& aStyleFont). Then, it's easier to maintain.

>@@ -452,16 +462,18 @@ public:
>     // true if selection is reversed (end < start)
>     bool mReversed;
>     // true if the selection exists
>     bool mHasSelection;
>     // true if DOM element under mouse belongs to widget
>     bool mWidgetIsHit;
>     // used by NS_QUERY_SELECTION_AS_TRANSFERABLE
>     nsCOMPtr<nsITransferable> mTransferable;
>+    // used by NS_QUERY_TEXT_CONTENT with font ranges requested
>+    nsTArray<mozilla::FontRange> mFontRanges;


I still think that this should be nsAutoTArray. Why do you keep this?
Attachment #8521910 - Flags: review?(masayuki) → review-
(Assignee)

Comment 29

4 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #28)
> Comment on attachment 8521910 [details] [diff] [review]
> patch
> 
> >+    int32_t lastXPEndOffset = frameXPStart;
> >+    while (runIter.NextRun()) {
> >+      gfxFont* font = runIter.GetGlyphRun()->mFont.get();
> >+      int32_t startXPOffset =
> >+        iter.ConvertSkippedToOriginal(runIter.GetStringStart());
> >+      // It is possible that the first glyph run has exceeded the frame,
> >+      // because the whole frame is filled by skipped chars.
> >+      if (startXPOffset >= frameXPEnd) {
> >+        break;
> >+      }
> >+
> >+      if (startXPOffset > lastXPEndOffset) {
> >+        // Create range for skipped leading chars.
> >+        baseOffset += AppendEmptyFontRange(aFontRanges, aContent, baseOffset,
> >+                                           lastXPEndOffset, startXPOffset,
> >+                                           aLineBreakType);
> 
> Perhaps, shouldn't you expand the previous range rather than appending empty
> font range? Or expand the range of next range? If so, widget doesn't meat
> "odd" font range. (but I'm not sure)

They are just skipped chars, which won't be displayed at all. I don't think we should give them any style.

> >+    if (lastXPEndOffset < frameXPEnd) {
> >+      // Create range for skipped trailing chars. It also handles case
> >+      // that the whole frame contains only skipped chars.
> >+      baseOffset += AppendEmptyFontRange(aFontRanges, aContent, baseOffset,
> >+                                         lastXPEndOffset, frameXPEnd,
> >+                                         aLineBreakType);
> 
> ditto.

ditto.

> >+/* static */ nsresult
> >+ContentEventHandler::GenerateFlatFontRanges(nsRange* aRange,
> >+                                            nsTArray<FontRange>& aFontRanges,
> >+                                            int32_t& aLength,
> >+                                            LineBreakType aLineBreakType)
> >+{
> >+  MOZ_ASSERT(aFontRanges.IsEmpty(), "aRanges must be empty array");
> >+
> >+  nsINode* startNode = aRange->GetStartParent();
> >+  nsINode* endNode = aRange->GetEndParent();
> >+  if (NS_WARN_IF(!startNode || !endNode)) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> >+
> >+  // baseOffset is the flattened offset of each content node.
> >+  int32_t baseOffset = 0;
> >+  nsCOMPtr<nsIContentIterator> iter = NS_NewContentIterator();
> >+  for (iter->Init(aRange); !iter->IsDone(); iter->Next()) {
> >+    nsINode* node = iter->GetCurrentNode();
> >+    if (!node) {
> 
> NS_WARN_IF()?

I don't know. I copied from other functions in that file.

> >+    if (content->IsNodeOfType(nsINode::eTEXT)) {
> >+      int32_t startOffset = content != startNode ? 0 : aRange->StartOffset();
> >+      int32_t endOffset = content != endNode ?
> >+        content->TextLength() : aRange->EndOffset();
> >+      baseOffset += AppendFontRanges(aFontRanges, content,
> >+                                     baseOffset, startOffset, endOffset,
> >+                                     aLineBreakType);
> >+    } else if (IsContentBR(content)) {
> >+      if (aFontRanges.IsEmpty()) {
> >+        MOZ_ASSERT(baseOffset == 0);
> >+        FontRange* fontRange = aFontRanges.AppendElement();
> >+        nsIFrame* brFrame = content->GetPrimaryFrame();
> >+        nscoord fontSize = brFrame->StyleFont()->mFont.size;
> >+        fontRange->mFontSize = NSAppUnitsToIntPixels(fontSize,
> >+                                                     AppUnitsPerCSSPixel());
> 
> How about fontface and bold information? In most cases, authors specify font
> styles to its ancestor block element. So, it may be useful.

StyleFont()->mFont.size is the computed font size of the give frame. No addition computation is needed IMHO.

Does it make sense to have fontface on br? I don't think so.

> >+  if (aEvent->mWithFontRanges) {
> >+    int32_t fontRangeLength;
> >+    rv = GenerateFlatFontRanges(range, aEvent->mReply.mFontRanges,
> >+                                fontRangeLength, lineBreakType);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> nit:
> 
> if (NS_WARNI_F(NS_FAILED(rv))) {
>   return rv;
> }

I'm not quite sure about the usage of NS_WARN_IF and NS_ENSURE_SUCCESS. Could you explain a bit more? Or is there any related document?

> >+struct FontRange
> >+{
> >+  FontRange()
> >+    : mStartOffset(0)
> >+    , mFontSize(0)
> >+  {
> >+  }
> >+
> >+  int32_t mStartOffset;
> >+  nsAutoString mFontName;
> >+  gfxFloat mFontSize; // in pixels
> >+};
> 
> Wow, mIsBold isn't necessary? And also how about aIsItalic?

In fact, bold and italic information are all included in the mFontName. Previously I add bold info, so that text with fonts that cannot be recognized by the system (e.g. user font defined by @font-face), can still have a similar weight.
But two reasons make me abandon it later:
1. Safari doesn't do so.
2. Bold information isn't that reliable. Sometimes a light user font has a bold weight. The similarity of display effect isn't actually improved by providing this information.

> >@@ -452,16 +462,18 @@ public:
> >     // true if selection is reversed (end < start)
> >     bool mReversed;
> >     // true if the selection exists
> >     bool mHasSelection;
> >     // true if DOM element under mouse belongs to widget
> >     bool mWidgetIsHit;
> >     // used by NS_QUERY_SELECTION_AS_TRANSFERABLE
> >     nsCOMPtr<nsITransferable> mTransferable;
> >+    // used by NS_QUERY_TEXT_CONTENT with font ranges requested
> >+    nsTArray<mozilla::FontRange> mFontRanges;
> 
> I still think that this should be nsAutoTArray. Why do you keep this?

We don't have ParamTraits for nsAutoTArray, and I don't think it is a good idea to have one. nsAutoTArray has two parameter, and the second one is an arbitrary number, which means we will always have a new specialization every time we use it. It blows up the size of the exectuable file with little performance benefits here.

In addition, according to my observation, OS X always do this when querying attributed string: query length 1; query length 2000; query the actual word length. Hence the length of font range can vary from 1 to hundreds. I don't think it makes any sense to give it a predefined local length. It won't help at all.
(Assignee)

Comment 30

3 years ago
Created attachment 8539683 [details] [diff] [review]
patch
Attachment #8521910 - Attachment is obsolete: true
Attachment #8539683 - Flags: review?(masayuki)
Oops, I'm sorry. I failed to catch your reply.

> > >+    int32_t lastXPEndOffset = frameXPStart;
> > >+    while (runIter.NextRun()) {
> > >+      gfxFont* font = runIter.GetGlyphRun()->mFont.get();
> > >+      int32_t startXPOffset =
> > >+        iter.ConvertSkippedToOriginal(runIter.GetStringStart());
> > >+      // It is possible that the first glyph run has exceeded the frame,
> > >+      // because the whole frame is filled by skipped chars.
> > >+      if (startXPOffset >= frameXPEnd) {
> > >+        break;
> > >+      }
> > >+
> > >+      if (startXPOffset > lastXPEndOffset) {
> > >+        // Create range for skipped leading chars.
> > >+        baseOffset += AppendEmptyFontRange(aFontRanges, aContent, baseOffset,
> > >+                                           lastXPEndOffset, startXPOffset,
> > >+                                           aLineBreakType);
> > 
> > Perhaps, shouldn't you expand the previous range rather than appending empty
> > font range? Or expand the range of next range? If so, widget doesn't meat
> > "odd" font range. (but I'm not sure)
> 
> They are just skipped chars, which won't be displayed at all. I don't think
> we should give them any style.

If all ranges are covered, i.e., not skipped, the user (typically, widget) doesn't need to check it and might not need to handle specially. So, not skipping any characters may help widget and the code simpler. 

> > >+/* static */ nsresult
> > >+ContentEventHandler::GenerateFlatFontRanges(nsRange* aRange,
> > >+                                            nsTArray<FontRange>& aFontRanges,
> > >+                                            int32_t& aLength,
> > >+                                            LineBreakType aLineBreakType)
> > >+{
> > >+  MOZ_ASSERT(aFontRanges.IsEmpty(), "aRanges must be empty array");
> > >+
> > >+  nsINode* startNode = aRange->GetStartParent();
> > >+  nsINode* endNode = aRange->GetEndParent();
> > >+  if (NS_WARN_IF(!startNode || !endNode)) {
> > >+    return NS_ERROR_FAILURE;
> > >+  }
> > >+
> > >+  // baseOffset is the flattened offset of each content node.
> > >+  int32_t baseOffset = 0;
> > >+  nsCOMPtr<nsIContentIterator> iter = NS_NewContentIterator();
> > >+  for (iter->Init(aRange); !iter->IsDone(); iter->Next()) {
> > >+    nsINode* node = iter->GetCurrentNode();
> > >+    if (!node) {
> > 
> > NS_WARN_IF()?
> 
> I don't know. I copied from other functions in that file.

I think that it shouldn't occur. If I were you, I'd use it and check it wouldn't be a spammer.

> > >+    if (content->IsNodeOfType(nsINode::eTEXT)) {
> > >+      int32_t startOffset = content != startNode ? 0 : aRange->StartOffset();
> > >+      int32_t endOffset = content != endNode ?
> > >+        content->TextLength() : aRange->EndOffset();
> > >+      baseOffset += AppendFontRanges(aFontRanges, content,
> > >+                                     baseOffset, startOffset, endOffset,
> > >+                                     aLineBreakType);
> > >+    } else if (IsContentBR(content)) {
> > >+      if (aFontRanges.IsEmpty()) {
> > >+        MOZ_ASSERT(baseOffset == 0);
> > >+        FontRange* fontRange = aFontRanges.AppendElement();
> > >+        nsIFrame* brFrame = content->GetPrimaryFrame();
> > >+        nscoord fontSize = brFrame->StyleFont()->mFont.size;
> > >+        fontRange->mFontSize = NSAppUnitsToIntPixels(fontSize,
> > >+                                                     AppUnitsPerCSSPixel());
> > 
> > How about fontface and bold information? In most cases, authors specify font
> > styles to its ancestor block element. So, it may be useful.
> 
> StyleFont()->mFont.size is the computed font size of the give frame. No
> addition computation is needed IMHO.

Okay.

> Does it make sense to have fontface on br? I don't think so.

If you set font information for <br>, it may help widget. For example, when there is only <br> element in the focused editor and IME or something attempts to retrieve fontface for its UI rendering, the fontface information is necessary.

> > >+  if (aEvent->mWithFontRanges) {
> > >+    int32_t fontRangeLength;
> > >+    rv = GenerateFlatFontRanges(range, aEvent->mReply.mFontRanges,
> > >+                                fontRangeLength, lineBreakType);
> > >+    NS_ENSURE_SUCCESS(rv, rv);
> > 
> > nit:
> > 
> > if (NS_WARNI_F(NS_FAILED(rv))) {
> >   return rv;
> > }
> 
> I'm not quite sure about the usage of NS_WARN_IF and NS_ENSURE_SUCCESS.
> Could you explain a bit more? Or is there any related document?

Basically, please use NS_WARN_IF() in new code. NS_ENSURE_* are legacy macro.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros

However, some reviewers don't need new NS_WARN_IF() style. If reviewers you requested like better NS_ENSURE_*, should use them, but otherwise, we should use the our standard coding rules.

> > >+struct FontRange
> > >+{
> > >+  FontRange()
> > >+    : mStartOffset(0)
> > >+    , mFontSize(0)
> > >+  {
> > >+  }
> > >+
> > >+  int32_t mStartOffset;
> > >+  nsAutoString mFontName;
> > >+  gfxFloat mFontSize; // in pixels
> > >+};
> > 
> > Wow, mIsBold isn't necessary? And also how about aIsItalic?
> 
> In fact, bold and italic information are all included in the mFontName.
> Previously I add bold info, so that text with fonts that cannot be
> recognized by the system (e.g. user font defined by @font-face), can still
> have a similar weight.
> But two reasons make me abandon it later:
> 1. Safari doesn't do so.
> 2. Bold information isn't that reliable. Sometimes a light user font has a
> bold weight. The similarity of display effect isn't actually improved by
> providing this information.

Hmm, I think that it's better to have their information. However, as you said, currently, it's not necessary and I have no idea for other purpose. Okay, let's ignore them for now.

> > >@@ -452,16 +462,18 @@ public:
> > >     // true if selection is reversed (end < start)
> > >     bool mReversed;
> > >     // true if the selection exists
> > >     bool mHasSelection;
> > >     // true if DOM element under mouse belongs to widget
> > >     bool mWidgetIsHit;
> > >     // used by NS_QUERY_SELECTION_AS_TRANSFERABLE
> > >     nsCOMPtr<nsITransferable> mTransferable;
> > >+    // used by NS_QUERY_TEXT_CONTENT with font ranges requested
> > >+    nsTArray<mozilla::FontRange> mFontRanges;
> > 
> > I still think that this should be nsAutoTArray. Why do you keep this?
> 
> We don't have ParamTraits for nsAutoTArray, and I don't think it is a good
> idea to have one. nsAutoTArray has two parameter, and the second one is an
> arbitrary number, which means we will always have a new specialization every
> time we use it. It blows up the size of the exectuable file with little
> performance benefits here.
> 
> In addition, according to my observation, OS X always do this when querying
> attributed string: query length 1; query length 2000; query the actual word
> length. Hence the length of font range can vary from 1 to hundreds. I don't
> think it makes any sense to give it a predefined local length. It won't help
> at all.

Okay. Then, keep using nsTArray.

I'll check your new patch if I have a change to do it today. But if it fails, I can review this Wendsday because tomorrow is a notional holiday of Japan.
(Assignee)

Comment 32

3 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31)
> 
> > > Perhaps, shouldn't you expand the previous range rather than appending empty
> > > font range? Or expand the range of next range? If so, widget doesn't meat
> > > "odd" font range. (but I'm not sure)
> > 
> > They are just skipped chars, which won't be displayed at all. I don't think
> > we should give them any style.
> 
> If all ranges are covered, i.e., not skipped, the user (typically, widget)
> doesn't need to check it and might not need to handle specially. So, not
> skipping any characters may help widget and the code simpler. 

It won't make anything simpler to give the information to every range, because at least, user-defined fontface could produce invalid font info. So widget should always be able to handle invalid font info.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #32)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31)
> > 
> > > > Perhaps, shouldn't you expand the previous range rather than appending empty
> > > > font range? Or expand the range of next range? If so, widget doesn't meat
> > > > "odd" font range. (but I'm not sure)
> > > 
> > > They are just skipped chars, which won't be displayed at all. I don't think
> > > we should give them any style.
> > 
> > If all ranges are covered, i.e., not skipped, the user (typically, widget)
> > doesn't need to check it and might not need to handle specially. So, not
> > skipping any characters may help widget and the code simpler. 
> 
> It won't make anything simpler to give the information to every range,
> because at least, user-defined fontface could produce invalid font info. So
> widget should always be able to handle invalid font info.

I agree with for the invalid fontfaces for the system.
Comment on attachment 8539683 [details] [diff] [review]
patch

Sorry for the delay.

>+/* static */ int32_t
>+ContentEventHandler::AppendFontRanges(nsTArray<FontRange>& aFontRanges,
>+                                      nsIContent* aContent,
>+                                      int32_t aBaseOffset,
>+                                      int32_t aXPStartOffset,
>+                                      int32_t aXPEndOffset,
>+                                      LineBreakType aLineBreakType)
>+{

I'm not familiar with around layout and gfx. Please request additional review for this method.

>+  int32_t baseOffset = aBaseOffset;
>+  nsTextFrame* curr = do_QueryFrame(frame);

nit: I like currentTextFrame.

>+  MOZ_ASSERT(curr, "Not a text frame");
>+  while (curr) {
>+    int32_t frameXPStart = std::max(curr->GetContentOffset(), aXPStartOffset);
>+    int32_t frameXPEnd = std::min(curr->GetContentEnd(), aXPEndOffset);
>+    if (frameXPStart >= frameXPEnd) {
>+      curr = static_cast<nsTextFrame*>(curr->GetNextContinuation());
>+      continue;
>+    }
>+
>+    gfxSkipCharsIterator iter = curr->EnsureTextRun(nsTextFrame::eInflated);
>+    gfxTextRun* textRun = curr->GetTextRun(nsTextFrame::eInflated);
>+    MOZ_ASSERT(textRun);

nit: Add message to the MOZ_ASSERT().

>+    nsTextFrame* next = nullptr;

nit: I like nextTextFrame.

>+/* static */ nsresult
>+ContentEventHandler::GenerateFlatFontRanges(nsRange* aRange,
>+                                            nsTArray<FontRange>& aFontRanges,
>+                                            int32_t& aLength,
>+                                            LineBreakType aLineBreakType)
>+{
>+  MOZ_ASSERT(aFontRanges.IsEmpty(), "aRanges must be empty array");
>+
>+  nsINode* startNode = aRange->GetStartParent();
>+  nsINode* endNode = aRange->GetEndParent();
>+  if (NS_WARN_IF(!startNode || !endNode)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  // baseOffset is the flattened offset of each content node.
>+  int32_t baseOffset = 0;
>+  nsCOMPtr<nsIContentIterator> iter = NS_NewContentIterator();
>+  for (iter->Init(aRange); !iter->IsDone(); iter->Next()) {
>+    nsINode* node = iter->GetCurrentNode();
>+    if (!node) {

As I said, please use NS_WARN_IF() if it wouldn't be a spammer.

>+    if (content->IsNodeOfType(nsINode::eTEXT)) {
>+      int32_t startOffset = content != startNode ? 0 : aRange->StartOffset();
>+      int32_t endOffset = content != endNode ?
>+        content->TextLength() : aRange->EndOffset();
>+      baseOffset += AppendFontRanges(aFontRanges, content,
>+                                     baseOffset, startOffset, endOffset,
>+                                     aLineBreakType);
>+    } else if (IsContentBR(content)) {
>+      if (aFontRanges.IsEmpty()) {
>+        MOZ_ASSERT(baseOffset == 0);
>+        FontRange* fontRange = aFontRanges.AppendElement();
>+        nsIFrame* brFrame = content->GetPrimaryFrame();
>+        nscoord fontSize = brFrame->StyleFont()->mFont.size;
>+        fontRange->mFontSize = NSAppUnitsToIntPixels(fontSize,
>+                                                     AppUnitsPerCSSPixel());

As I said, I still think that fontface may be useful for IME if caret is surrounded by <br> elements when IME queries font information. Although, following case will cause strange behavior...

br {
  font-family: sans-serif;
}

p {
  font-family: serif;
}

<p contenteditable>
<br>
</p>

Is it better to use parent content's primary frame font information?

>+  // Append a font range with no font info for non-rendered text.
>+  // Return the length the range covers.
>+  static
>+    int32_t AppendEmptyFontRange(nsTArray<mozilla::FontRange>& aFontRanges,

Hmm, if nsTArray<mozilla::FontRange> is too long for our coding rules, you can use typedef in FontRange.h.

E.g., typedef nsTArray<mozilla::FontRange> FontRangeArray;

>@@ -588,16 +598,22 @@ struct ParamTraits<InfallibleTArray<E> >
>   }
> 
>   static void Log(const paramType& aParam, std::wstring* aLog)
>   {
>     LogParam(static_cast<const FallibleTArray<E>&>(aParam), aLog);
>   }
> };
> 
>+template<typename E, size_t N>
>+struct ParamTraits<nsAutoTArray<E, N>> : ParamTraits<nsTArray<E>>
>+{
>+  typedef nsAutoTArray<E, N> paramType;
>+};

As you said, if it's not worthwhile to use nsAutoTArray, please remove this and use nsTArray.

>@@ -488,16 +498,18 @@ public:
>     // true if the selection exists
>     bool mHasSelection;
>     // true if DOM element under mouse belongs to widget
>     bool mWidgetIsHit;
>     // mozilla::WritingMode value at the end (focus) of the selection
>     mozilla::WritingMode mWritingMode;
>     // used by NS_QUERY_SELECTION_AS_TRANSFERABLE
>     nsCOMPtr<nsITransferable> mTransferable;
>+    // used by NS_QUERY_TEXT_CONTENT with font ranges requested
>+    nsAutoTArray<mozilla::FontRange, 1> mFontRanges;

If you keep using nsAutoTArray for this, it doesn't make sense to use 1.

>@@ -2970,33 +2970,51 @@ IMEInputHandler::GetAttributedSubstringF
>   NSString* nsstr = nsCocoaUtils::ToNSString(textContent.mReply.mString);
>-  NSAttributedString* result =
>-    [[[NSAttributedString alloc] initWithString:nsstr
>-                                     attributes:nil] autorelease];
>+  NSMutableAttributedString* result =
>+    [[[NSMutableAttributedString alloc] initWithString:nsstr
>+                                            attributes:nil] autorelease];
>+  const nsTArray<FontRange>& fontRanges = textContent.mReply.mFontRanges;
>+  int32_t lastOffset = textContent.mReply.mString.Length();
>+  for (auto i = fontRanges.Length(); i > 0; --i) {
>+    const FontRange& fontRange = fontRanges[i - 1];
>+    NSString* fontName = nsCocoaUtils::ToNSString(fontRange.mFontName);
>+    CGFloat fontSize = fontRange.mFontSize / mWidget->BackingScaleFactor();
>+    NSFont* font = [NSFont fontWithName:fontName size:fontSize];
>+    if (!font) {
>+      font = [NSFont systemFontOfSize:fontSize];
>+    }
>+
>+    NSDictionary* attrs = @{ NSFontAttributeName: font };
>+    NSRange range = NSMakeRange(fontRange.mStartOffset,
>+                                lastOffset - fontRange.mStartOffset);
>+    [result setAttributes:attrs range:range];
>+    lastOffset = fontRange.mStartOffset;
>+  }

Please request review to smichaud for here.

I'd like to check next patch, especially around the style of <br>.
Attachment #8539683 - Flags: review?(masayuki)
(Assignee)

Comment 35

3 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #34)
> Comment on attachment 8539683 [details] [diff] [review]
> patch
> 
> >+  int32_t baseOffset = aBaseOffset;
> >+  nsTextFrame* curr = do_QueryFrame(frame);
> 
> nit: I like currentTextFrame.

I don't like that long name. It is obvious that this method is almost all about text frames. And it is actually a loop variable. Just like you won't use "contentIter" instead of "iter", or "xxxArrayIndex" instead of "i", I don't think it is necessary to replace this short name with that necessarily long name.

> >+  MOZ_ASSERT(curr, "Not a text frame");
> >+  while (curr) {
> >+    int32_t frameXPStart = std::max(curr->GetContentOffset(), aXPStartOffset);
> >+    int32_t frameXPEnd = std::min(curr->GetContentEnd(), aXPEndOffset);
> >+    if (frameXPStart >= frameXPEnd) {
> >+      curr = static_cast<nsTextFrame*>(curr->GetNextContinuation());
> >+      continue;
> >+    }
> >+
> >+    gfxSkipCharsIterator iter = curr->EnsureTextRun(nsTextFrame::eInflated);
> >+    gfxTextRun* textRun = curr->GetTextRun(nsTextFrame::eInflated);
> >+    MOZ_ASSERT(textRun);
> 
> nit: Add message to the MOZ_ASSERT().

Given that we called EnsureTextRun, I guess it is probably fine to remove this MOZ_ASSERT.

> >+    nsTextFrame* next = nullptr;
> 
> nit: I like nextTextFrame.

I don't like. ditto.

> As you said, if it's not worthwhile to use nsAutoTArray, please remove this
> and use nsTArray.
> 
> >@@ -488,16 +498,18 @@ public:
> >     // true if the selection exists
> >     bool mHasSelection;
> >     // true if DOM element under mouse belongs to widget
> >     bool mWidgetIsHit;
> >     // mozilla::WritingMode value at the end (focus) of the selection
> >     mozilla::WritingMode mWritingMode;
> >     // used by NS_QUERY_SELECTION_AS_TRANSFERABLE
> >     nsCOMPtr<nsITransferable> mTransferable;
> >+    // used by NS_QUERY_TEXT_CONTENT with font ranges requested
> >+    nsAutoTArray<mozilla::FontRange, 1> mFontRanges;
> 
> If you keep using nsAutoTArray for this, it doesn't make sense to use 1.

It make sense. As I said, in most cases (roughly 2/3 in content query for dictionary overlay) this array contains only one element. Making it nsAutoTArray<, 1> allows us not to allocate in-heap space for that only element.
(Assignee)

Comment 36

3 years ago
Created attachment 8550675 [details] [diff] [review]
patch 1 - Support getting font info in content query.
Attachment #8550675 - Flags: review?(masayuki)
Attachment #8550675 - Flags: review?(bugs)
(Assignee)

Comment 37

3 years ago
Created attachment 8550676 [details] [diff] [review]
patch 2 - Provide font info for content query on Mac.
Attachment #8539683 - Attachment is obsolete: true
Attachment #8550676 - Flags: review?(smichaud)
Comment on attachment 8550675 [details] [diff] [review]
patch 1 - Support getting font info in content query.

>+  int32_t baseOffset = aBaseOffset;
>+  nsTextFrame* curr = do_QueryFrame(frame);

I sill don't think this is good name. At reading this code not from begging of this method, the name, |curr|, needs to check its type. Longer name isn't so bad here because it doesn't cause so a lot of broken lines. So, please user crrentTextFrame. (I believe that longer name isn't so bad if it makes the code clearer except when the name is too much long for 80 characters per line rule)

> I don't like that long name. It is obvious that this method is almost all
> about text frames. And it is actually a loop variable. Just like you won't
> use "contentIter" instead of "iter", or "xxxArrayIndex" instead of "i", I
> don't think it is necessary to replace this short name with that necessarily
> long name.

Yes, |iter| isn't a good name as you said. I agree with you. But using |i|, |j| etc for counter in loops isn't bad name because it's traditional rules, i.e., most developers know such variables names are int or uint. I agree with that it's not the best names, though.

>+    nsTextFrame* next = nullptr;

Similarly, I still think this should be nextTextFrame.

>@@ -692,16 +859,28 @@ ContentEventHandler::OnQueryTextContent(
>   rv = SetRangeFromFlatTextOffset(range, aEvent->mInput.mOffset,
>                                   aEvent->mInput.mLength, lineBreakType, false,
>                                   &aEvent->mReply.mOffset);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = GenerateFlatTextContent(range, aEvent->mReply.mString, lineBreakType);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  if (aEvent->mWithFontRanges) {
>+    int32_t fontRangeLength;
>+    rv = GenerateFlatFontRanges(range, aEvent->mReply.mFontRanges,
>+                                fontRangeLength, lineBreakType);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+
>+    NS_ASSERTION(fontRangeLength == int32_t(aEvent->mReply.mString.Length()),

Hmm, I like static_cast<int32_t>() better since C++ cast style is better when developers search casting code. However, the rule has been dropped from our coding style guide, so, it's okay to stay this... But if I were you, I'd use C++ cast style here.

>@@ -488,16 +498,18 @@ public:
>     // true if the selection exists
>     bool mHasSelection;
>     // true if DOM element under mouse belongs to widget
>     bool mWidgetIsHit;
>     // mozilla::WritingMode value at the end (focus) of the selection
>     mozilla::WritingMode mWritingMode;
>     // used by NS_QUERY_SELECTION_AS_TRANSFERABLE
>     nsCOMPtr<nsITransferable> mTransferable;
>+    // used by NS_QUERY_TEXT_CONTENT with font ranges requested
>+    nsAutoTArray<mozilla::FontRange, 1> mFontRanges;

If remaining 1/3 cases need a lot of items, as you said, it's okay this only allocate only one room for the only item.


The others look okay to me.
Attachment #8550675 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #38)
> I sill don't think this is good name. At reading this code not from begging
> of this method, the name, |curr|, needs to check its type. Longer name isn't
> so bad here because it doesn't cause so a lot of broken lines. So, please
> user crrentTextFrame.
currentTextFrame ;)
Comment on attachment 8550675 [details] [diff] [review]
patch 1 - Support getting font info in content query.

>+static inline uint32_t
>+GetBRLength(LineBreakType aLineBreakType)
>+{
>+#if defined(XP_WIN)
>+  // Length of \r\n
>+  return (aLineBreakType == LINE_BREAK_TYPE_NATIVE) ? 2 : 1;
>+#else
>+  return 1;
>+#endif
>+}
>+
> /* static */ uint32_t
> ContentEventHandler::GetTextLength(nsIContent* aContent,
>                                    LineBreakType aLineBreakType,
>                                    uint32_t aMaxLength)
> {
>   if (aContent->IsNodeOfType(nsINode::eTEXT)) {
>     uint32_t textLengthDifference =
> #if defined(XP_MACOSX)
>@@ -339,22 +350,17 @@ ContentEventHandler::GetTextLength(nsICo
> 
>     const nsTextFragment* text = aContent->GetText();
>     if (!text) {
>       return 0;
>     }
>     uint32_t length = std::min(text->GetLength(), aMaxLength);
>     return length + textLengthDifference;
>   } else if (IsContentBR(aContent)) {
>-#if defined(XP_WIN)
>-    // Length of \r\n
>-    return (aLineBreakType == LINE_BREAK_TYPE_NATIVE) ? 2 : 1;
>-#else
>-    return 1;
>-#endif
>+    return GetBRLength(aLineBreakType);
>   }
>   return 0;
> }
> 
> static uint32_t ConvertToXPOffset(nsIContent* aContent, uint32_t aNativeOffset)
> {
> #if defined(XP_MACOSX)
>   // On Mac, the length of a native newline ("\r") is equal to the length of
>@@ -388,17 +394,16 @@ static nsresult GenerateFlatTextContent(
>   if (startNode == endNode && startNode->IsNodeOfType(nsINode::eTEXT)) {
>     nsIContent* content = static_cast<nsIContent*>(startNode);
>     AppendSubString(aString, content, aRange->StartOffset(),
>                     aRange->EndOffset() - aRange->StartOffset());
>     ConvertToNativeNewlines(aString);
>     return NS_OK;
>   }
> 
>-  nsAutoString tmpStr;
>   for (; !iter->IsDone(); iter->Next()) {
>     nsINode* node = iter->GetCurrentNode();
>     if (!node) {
>       break;
>     }
>     if (!node->IsNodeOfType(nsINode::eCONTENT)) {
>       continue;
>     }
>@@ -418,16 +423,178 @@ static nsresult GenerateFlatTextContent(
>     }
>   }
>   if (aLineBreakType == LINE_BREAK_TYPE_NATIVE) {
>     ConvertToNativeNewlines(aString);
>   }
>   return NS_OK;
> }
> 
>+/* static */ int32_t
>+ContentEventHandler::AppendEmptyFontRange(FontRangeArray& aFontRanges,
>+                                          nsIContent* aContent,
>+                                          int32_t aBaseOffset,
>+                                          int32_t aXPStartOffset,
>+                                          int32_t aXPEndOffset,
>+                                          LineBreakType aLineBreakType)
>+{
>+  FontRange* fontRange = aFontRanges.AppendElement();
>+  fontRange->mStartOffset = aBaseOffset;
>+  return aLineBreakType == LINE_BREAK_TYPE_NATIVE ?
>+    GetNativeTextLength(aContent, aXPStartOffset, aXPEndOffset) :
>+    aXPEndOffset - aXPStartOffset;
>+}
>+
>+/* static */ int32_t
>+ContentEventHandler::AppendFontRanges(FontRangeArray& aFontRanges,
>+                                      nsIContent* aContent,
>+                                      int32_t aBaseOffset,
>+                                      int32_t aXPStartOffset,
>+                                      int32_t aXPEndOffset,
>+                                      LineBreakType aLineBreakType)
this method needs layout peer review.



> 
>+  if (aEvent->mWithFontRanges) {
>+    int32_t fontRangeLength;
>+    rv = GenerateFlatFontRanges(range, aEvent->mReply.mFontRanges,
>+                                fontRangeLength, lineBreakType);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return rv;
>+    }
>+
>+    NS_ASSERTION(fontRangeLength == int32_t(aEvent->mReply.mString.Length()),
>+                 "Font ranges doesn't match the string");
You want MOZ_ASSERT



>+  // Append a font range with no font info for non-rendered text.
>+  // Return the length the range covers.
>+  static int32_t AppendEmptyFontRange(FontRangeArray& aFontRanges,
>+                                      nsIContent* aContent,
>+                                      int32_t aBaseOffset,
>+                                      int32_t aXPStartOffset,
>+                                      int32_t aXPEndOffset,
>+                                      LineBreakType aLineBreakType);
The method name is quite weird. It appends empty font range sure, but
then it also calculates something and returns that. And the return value
has nothing to do with the empty font range.
Could you just split this to two.
void AppendEmptyFontRange();
int32_t CalculateTextLengthInRange(...);

or rename the method so that the name indicates what the method actually does.



>+  // Append font ranges, return the total length the added ranges cover.
>+  static int32_t AppendFontRanges(FontRangeArray& aFontRanges,
>+                                  nsIContent* aContent,
>+                                  int32_t aBaseOffset,
>+                                  int32_t aXPStartOffset,
>+                                  int32_t aXPEndOffset,
>+                                  LineBreakType aLineBreakType);
Similar naming issue with this.

>+  static nsresult GenerateFlatFontRanges(nsRange* aRange,
What does "Flat" mean in this context?

>+#ifdef MOZILLA_INTERNAL_API
>+
>+template<>
>+struct ParamTraits<nsAutoString> : ParamTraits<nsString>
>+{
>+  typedef nsAutoString paramType;
>+};
>+
>+#endif // MOZILLA_INTERNAL_API
Please stick with nsString if possible.



>+struct FontRange
>+{
>+  FontRange()
>+    : mStartOffset(0)
>+    , mFontSize(0)
>+  {
>+  }
>+
>+  int32_t mStartOffset;
>+  nsAutoString mFontName;
nsString mFontName
Attachment #8550675 - Flags: review?(bugs) → review-
(Assignee)

Comment 41

3 years ago
(In reply to Olli Pettay [:smaug] from comment #40)
> Comment on attachment 8550675 [details] [diff] [review]
> patch 1 - Support getting font info in content query.
> 
> >+struct FontRange
> >+{
> >+  FontRange()
> >+    : mStartOffset(0)
> >+    , mFontSize(0)
> >+  {
> >+  }
> >+
> >+  int32_t mStartOffset;
> >+  nsAutoString mFontName;
> nsString mFontName

In comment 19, masayuki told me nsAutoString might be better... You two probably want to discuss a bit about which is better. I'm fine with either way, though.
Flags: needinfo?(masayuki)
nsString should be better. We assign some value to mFontName so there is a high chance that we'll
just end up sharing internal nsStringBuffer.
And if it is nsAutoString 
nsAutoTArray<mozilla::FontRange, 1> mFontRanges; becomes quite memory hungry, since just
that one item takes >64 bytes.
(In reply to Olli Pettay [:smaug] from comment #42)
> nsString should be better. We assign some value to mFontName so there is a
> high chance that we'll
> just end up sharing internal nsStringBuffer.
> And if it is nsAutoString 
> nsAutoTArray<mozilla::FontRange, 1> mFontRanges; becomes quite memory
> hungry, since just
> that one item takes >64 bytes.

So, using nsAutoString allocates 64 bytes initially. Doesn't it suppress additional allocation in most cases? (i.e., suppressing heap fragments when this is queried a lot of times) I'm not familiar with nsStringBuffer behavior, though.
Flags: needinfo?(masayuki) → needinfo?(bugs)
Well, nsAutoString takes the space all the time. And in this case it is very much
possible that nothing will be assigned to it. And when something is assigned, it is often
font->GetName(), which return nsString&. So it possibly points to a nsStringBuffer
(which is shared, refcounted data between string objects). So most probably
nsString ends up using less memory here.
Flags: needinfo?(bugs)
nsAutoString is good when used on stack, since it can either contain the string inside itself
(when short enough) or point to a stringbuffer etc. So usually no allocation/free needed.

But here we aren't dealing stack-only objects, since FontRange is kept alive in an array, which
will allocate from heap once the length > 1.
(In reply to Olli Pettay [:smaug] from comment #44)
> Well, nsAutoString takes the space all the time. And in this case it is very
> much
> possible that nothing will be assigned to it. And when something is
> assigned, it is often
> font->GetName(), which return nsString&. So it possibly points to a
> nsStringBuffer
> (which is shared, refcounted data between string objects). So most probably
> nsString ends up using less memory here.

Oh, then, it should be better to use nsString.

(In reply to Olli Pettay [:smaug] from comment #45)
> nsAutoString is good when used on stack, since it can either contain the
> string inside itself
> (when short enough) or point to a stringbuffer etc. So usually no
> allocation/free needed.
> 
> But here we aren't dealing stack-only objects, since FontRange is kept alive
> in an array, which
> will allocate from heap once the length > 1.

WidgetQueryContentEvent is usually created in stack. And I assume that in this case, the items which is allocated initially are allocated in stack too. Therefore, if the array should be defined with a number which is not too large but enough in most cases.
(Assignee)

Comment 47

3 years ago
(In reply to Olli Pettay [:smaug] from comment #40)
> Comment on attachment 8550675 [details] [diff] [review]
> patch 1 - Support getting font info in content query.
> 
> >+  // Append a font range with no font info for non-rendered text.
> >+  // Return the length the range covers.
> >+  static int32_t AppendEmptyFontRange(FontRangeArray& aFontRanges,
> >+                                      nsIContent* aContent,
> >+                                      int32_t aBaseOffset,
> >+                                      int32_t aXPStartOffset,
> >+                                      int32_t aXPEndOffset,
> >+                                      LineBreakType aLineBreakType);
> The method name is quite weird. It appends empty font range sure, but
> then it also calculates something and returns that. And the return value
> has nothing to do with the empty font range.
> Could you just split this to two.
> void AppendEmptyFontRange();
> int32_t CalculateTextLengthInRange(...);
> 
> or rename the method so that the name indicates what the method actually
> does.
> 
> >+  // Append font ranges, return the total length the added ranges cover.
> >+  static int32_t AppendFontRanges(FontRangeArray& aFontRanges,
> >+                                  nsIContent* aContent,
> >+                                  int32_t aBaseOffset,
> >+                                  int32_t aXPStartOffset,
> >+                                  int32_t aXPEndOffset,
> >+                                  LineBreakType aLineBreakType);
> Similar naming issue with this.

I think there is no naming issue with these two functions. They do append the font ranges, and then return the length of characters the appended ranges cover. I think the names along with the comments are clear enough. And I think it is a very usual pattern to write such kind of functions, which changes something and return some useful information about what it just changes. Think about fread(), which reads data into the buffer, and returns the length it reads.

> >+  static nsresult GenerateFlatFontRanges(nsRange* aRange,
> What does "Flat" mean in this context?

Like every "Flat"s else in that file, it means the offset is calculated based on flattened text extracted from the page.
(Assignee)

Comment 48

3 years ago
Comment on attachment 8550675 [details] [diff] [review]
patch 1 - Support getting font info in content query.

Jonathan, could you please review ContentEventHandler::AppendFontRanges?
Attachment #8550675 - Flags: review?(jfkthame)
(I wouldn't use C function names any good examples ;)).

But ok, fine.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #47)
> (In reply to Olli Pettay [:smaug] from comment #40)
> > The method name is quite weird. It appends empty font range sure, but
> > then it also calculates something and returns that. And the return value
> > has nothing to do with the empty font range.
> > Could you just split this to two.
> > void AppendEmptyFontRange();
> > int32_t CalculateTextLengthInRange(...);

> I think there is no naming issue with these two functions. They do append
> the font ranges, and then return the length of characters the appended
> ranges cover. I think the names along with the comments are clear enough.
> And I think it is a very usual pattern to write such kind of functions,
> which changes something and return some useful information about what it
> just changes. Think about fread(), which reads data into the buffer, and
> returns the length it reads.

I'm not a DOM peer, so it's not my place to decide things here, but FWIW... I think there's a difference here from the fread() example, in that fread()'s return value is an important indicator of what the function actually did: it may or may not have actually read the requested amount. In the case of AppendEmptyFontRange, OTOH, the computation of the length that's returned is independent of the primary purpose of the method. It does its thing (appending the range and setting its mStartOffset) based on two of the parameters, and then it calculates the returned length using the other four; there's nothing in common between these two operations.

As such, I'm at least somewhat in sympathy with :smaug's idea that it could be split into two methods, one for each of these tasks.

In fact, I'd suggest the simplified AppendEmptyFontRange() could be renamed to AppendFontRange(), and return a pointer to the newly-appended (and initially empty) range record. Then the loop in AppendFontRanges could also use this instead of directly calling AppendElement(), then set up the font fields in the new range that's returned.

As for AppendFontRanges, though, it's less clear to me. If we had a CalculateTextLengthInRange(...) method, would that provide the correct length to be used in GenerateFlatFontRanges, or are there cases where the result of AppendFontRanges (as currently written, returning |baseOffset - aBaseOffset|) would be different from this? If so, there may be a case for keeping the combined append-and-calculate-length functionality. In which case it could be called AppendFontRangesAndCalcuateTextLength().
Comment on attachment 8550675 [details] [diff] [review]
patch 1 - Support getting font info in content query.

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

Clearing r?jfkthame (not a peer). I'd suggest looking at whether a separation into Append... and CalculateLength... methods, as :smaug suggested, would work cleanly. If it's really problematic, though,

::: dom/events/ContentEventHandler.cpp
@@ +516,5 @@
> +        iter.ConvertSkippedToOriginal(runIter.GetStringEnd());
> +      endXPOffset = std::min(frameXPEnd, endXPOffset);
> +      baseOffset += aLineBreakType == LINE_BREAK_TYPE_NATIVE ?
> +        GetNativeTextLength(aContent, startXPOffset, endXPOffset) :
> +        endXPOffset - startXPOffset;

With the proposal to separate appending from length-calculation, we'd have a helper to use here, and this could become:

    baseOffset +=
      CalculateTextLengthInRange(aContent, startXPOffset, endXPOffset,
                                 aLineBreakType);

which I think reads much more easily, fwiw.
Attachment #8550675 - Flags: review?(jfkthame)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #47)
> (In reply to Olli Pettay [:smaug] from comment #40)
> > >+  static nsresult GenerateFlatFontRanges(nsRange* aRange,
> > What does "Flat" mean in this context?
> 
> Like every "Flat"s else in that file, it means the offset is calculated
> based on flattened text extracted from the page.

 We need to think contents as plaintext. ContentEventHandler generates string from text nodes and br nodes. The generated text is called "flat text".
(Assignee)

Comment 53

3 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #50)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #47)
> > (In reply to Olli Pettay [:smaug] from comment #40)
> > > The method name is quite weird. It appends empty font range sure, but
> > > then it also calculates something and returns that. And the return value
> > > has nothing to do with the empty font range.
> > > Could you just split this to two.
> > > void AppendEmptyFontRange();
> > > int32_t CalculateTextLengthInRange(...);
> 
> > I think there is no naming issue with these two functions. They do append
> > the font ranges, and then return the length of characters the appended
> > ranges cover. I think the names along with the comments are clear enough.
> > And I think it is a very usual pattern to write such kind of functions,
> > which changes something and return some useful information about what it
> > just changes. Think about fread(), which reads data into the buffer, and
> > returns the length it reads.
> 
> I'm not a DOM peer, so it's not my place to decide things here, but FWIW...

:smaug is a DOM peer, and he said this method needs review from a layout peer, which I think you are.

> I think there's a difference here from the fread() example, in that
> fread()'s return value is an important indicator of what the function
> actually did: it may or may not have actually read the requested amount. In
> the case of AppendEmptyFontRange, OTOH, the computation of the length that's
> returned is independent of the primary purpose of the method. It does its
> thing (appending the range and setting its mStartOffset) based on two of the
> parameters, and then it calculates the returned length using the other four;
> there's nothing in common between these two operations.

AppendEmptyFontRange is made this way mainly to match what AppendFontRanges does. I think it would be strange that two methods with similar behavior have very different usage.

> As such, I'm at least somewhat in sympathy with :smaug's idea that it could
> be split into two methods, one for each of these tasks.
> 
> In fact, I'd suggest the simplified AppendEmptyFontRange() could be renamed
> to AppendFontRange(), and return a pointer to the newly-appended (and
> initially empty) range record. Then the loop in AppendFontRanges could also
> use this instead of directly calling AppendElement(), then set up the font
> fields in the new range that's returned.

This suggestion sounds reasonable.

> As for AppendFontRanges, though, it's less clear to me. If we had a
> CalculateTextLengthInRange(...) method, would that provide the correct
> length to be used in GenerateFlatFontRanges, or are there cases where the
> result of AppendFontRanges (as currently written, returning |baseOffset -
> aBaseOffset|) would be different from this? If so, there may be a case for
> keeping the combined append-and-calculate-length functionality. In which
> case it could be called AppendFontRangesAndCalcuateTextLength().

I guess they are the same. OK, I probably need to reivew this patch myself again. It has been a long time since I first wrote it.
(Assignee)

Comment 54

3 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #38)
> Comment on attachment 8550675 [details] [diff] [review]
> patch 1 - Support getting font info in content query.
> 
> >+  int32_t baseOffset = aBaseOffset;
> >+  nsTextFrame* curr = do_QueryFrame(frame);
> 
> I sill don't think this is good name. At reading this code not from begging
> of this method, the name, |curr|, needs to check its type. Longer name isn't
> so bad here because it doesn't cause so a lot of broken lines. So, please
> user crrentTextFrame. (I believe that longer name isn't so bad if it makes
> the code clearer except when the name is too much long for 80 characters per
> line rule)

Actually I tried to use the name you proposed when I saw this comment. But later I found that, basically every line with this variable (and |next|) will need to be broken into two lines that way, which finally made me disagree and reply against this opinion.

I guess I probably have used some name similar to your proposal at the beginning, but then shortened it to make lines more compact.
(Assignee)

Comment 55

3 years ago
Created attachment 8554066 [details] [diff] [review]
patch 1 - Support getting font info in content query. r=masayuki
Attachment #8550675 - Attachment is obsolete: true
Attachment #8554066 - Flags: review?(jfkthame)
Attachment #8554066 - Flags: review?(bugs)
Comment on attachment 8554066 [details] [diff] [review]
patch 1 - Support getting font info in content query. r=masayuki

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

> :smaug is a DOM peer, and he said this method needs review from a layout peer, which I think you are.

Ah, right -- sorry, I overlooked that comment. Anyhow, this updated version looks OK to me, with a couple minor comments/questions below.

::: dom/events/ContentEventHandler.cpp
@@ +444,5 @@
> +{
> +  return aLineBreakType == LINE_BREAK_TYPE_NATIVE ?
> +    GetNativeTextLength(aContent, aXPStartOffset, aXPEndOffset) :
> +    aXPEndOffset - aXPStartOffset;
> +}

Is there any reason to expose this as a class method, or could it just be a static helper function in the .cpp file?

@@ +870,5 @@
> +    rv = GenerateFlatFontRanges(range, aEvent->mReply.mFontRanges,
> +                                fontRangeLength, lineBreakType);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }

If GenerateFlatFontRanges fails, it will have already emitted a warning; is it useful to warn again here?

::: widget/FontRange.h
@@ +18,5 @@
> +  }
> +
> +  int32_t mStartOffset;
> +  nsString mFontName;
> +  gfxFloat mFontSize; // in pixels

Device pixels or CSS pixels? (I'm guessing this means device pixels, but it would be good to make this explicit.)
Attachment #8554066 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 57

3 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #56)
> Comment on attachment 8554066 [details] [diff] [review]
> patch 1 - Support getting font info in content query. r=masayuki
> 
> Review of attachment 8554066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > :smaug is a DOM peer, and he said this method needs review from a layout peer, which I think you are.
> 
> Ah, right -- sorry, I overlooked that comment. Anyhow, this updated version
> looks OK to me, with a couple minor comments/questions below.
> 
> ::: dom/events/ContentEventHandler.cpp
> @@ +444,5 @@
> > +{
> > +  return aLineBreakType == LINE_BREAK_TYPE_NATIVE ?
> > +    GetNativeTextLength(aContent, aXPStartOffset, aXPEndOffset) :
> > +    aXPEndOffset - aXPStartOffset;
> > +}
> 
> Is there any reason to expose this as a class method, or could it just be a
> static helper function in the .cpp file?

Because GetNativeTextLength is a class method. Well, actually I'm fine with either way. I remember that it was an advice from masayuki. See comment 19, search |ContentEventHandler::|.

> @@ +870,5 @@
> > +    rv = GenerateFlatFontRanges(range, aEvent->mReply.mFontRanges,
> > +                                fontRangeLength, lineBreakType);
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +      return rv;
> > +    }
> 
> If GenerateFlatFontRanges fails, it will have already emitted a warning; is
> it useful to warn again here?

I don't know. Probably yes. This method also includes NS_ENSURE_SUCCESS, which I guess will also emit warnings. When I tried to use that macro, I was suggested to use NS_WARN_IF(NS_FAILED()) instead. So I guess it probably matches what the old code does.

> ::: widget/FontRange.h
> @@ +18,5 @@
> > +  }
> > +
> > +  int32_t mStartOffset;
> > +  nsString mFontName;
> > +  gfxFloat mFontSize; // in pixels
> 
> Device pixels or CSS pixels? (I'm guessing this means device pixels, but it
> would be good to make this explicit.)

I'm a bit confused here as well, and I probably have mixed them to some extent. What's the unit of gfxFont::GetAdjustedSize? I guess it's device pixels. But what about nsFont::size? I guess it's CSS pixels...
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #57)

> > > +  gfxFloat mFontSize; // in pixels
> > 
> > Device pixels or CSS pixels? (I'm guessing this means device pixels, but it
> > would be good to make this explicit.)
> 
> I'm a bit confused here as well, and I probably have mixed them to some
> extent. What's the unit of gfxFont::GetAdjustedSize? I guess it's device
> pixels.

Yes. And that's the right thing to be returning here, AFAICT. (Testing indicates that it gives the right result on a Retina Mac, where device pixels and CSS pixels are very different.)

> But what about nsFont::size? I guess it's CSS pixels...

Yes -- which means that the code for handling an initial <br> frame is probably wrong; I think it should be doing something more like

  fontRange->mFontSize = parentFrame->PresContext()->AppUnitsToDevPixels(font.size);

to get the devPixel size.
(Assignee)

Comment 59

3 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #58)
> Yes -- which means that the code for handling an initial <br> frame is
> probably wrong; I think it should be doing something more like
> 
>   fontRange->mFontSize =
> parentFrame->PresContext()->AppUnitsToDevPixels(font.size);
> 
> to get the devPixel size.

Thanks! Fixed this locally.
Comment on attachment 8554066 [details] [diff] [review]
patch 1 - Support getting font info in content query. r=masayuki

>+    if (!node->IsNodeOfType(nsINode::eCONTENT)) {
!node->IsContent()

>+    } else if (IsContentBR(content)) {
>+      if (aFontRanges.IsEmpty()) {
>+        MOZ_ASSERT(baseOffset == 0);
>+        FontRange* fontRange = AppendFontRange(aFontRanges, baseOffset);
>+        nsIFrame* parentFrame = content->GetPrimaryFrame()->GetParent();
Note, GetPrimaryFrame might return null here, so add a null check.
Attachment #8554066 - Flags: review?(bugs) → review+
(Assignee)

Comment 61

3 years ago
Ping. Could you review the Obj-C part please?
Flags: needinfo?(smichaud)
I frankly have been waiting for the other reviews to finish first :-)

Now that they have, I should be able to get to this tomorrow.
Flags: needinfo?(smichaud)
Comment on attachment 8550676 [details] [diff] [review]
patch 2 - Provide font info for content query on Mac.

This looks fine to me, though I haven't tested it.

I was unfamiliar with the NSDictionary literal syntax, so I had to look it up.  For reference here's Apple's doc:

https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/FoundationTypesandCollections/FoundationTypesandCollections.html#//apple_ref/doc/uid/TP40011210-CH7-SW25
Attachment #8550676 - Flags: review?(smichaud) → review+
https://hg.mozilla.org/mozilla-central/rev/bae702058e42
https://hg.mozilla.org/mozilla-central/rev/9758962dd013
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.