Closed Bug 1063857 Opened 9 years ago Closed 9 years ago

Spaces between characters are not equal for "text-align-last: justify"

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(8 files, 22 obsolete files)

screenshot
190.08 KB, image/png
Details
testcases
1.23 KB, text/html
Details
patch for test
3.14 KB, patch
masayuki
: review-
Details | Diff | Splinter Review
patch 2 - improve selecting, r=roc
5.10 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
patch, r=roc,masayuki
45.33 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
comparison for justification-cjk-extension
9.11 KB, image/png
Details
More intuitive test case for regression of bug 503399
1.66 KB, text/html
Details
patch for fixing reftests
9.27 KB, patch
roc
: review+
Details | Diff | Splinter Review 
Test code:
data:text/html;charset=utf-8,<p style="-moz-text-align-last: justify;">お早うございます</p>

In this case, the spaces between the characters are not equal. More precisely, there is no spacing surrounding the first and last character.

I guess it is in fact a duplicate of bug 424667.
Attached image screenshot — 
Difference between Firefox and Chrome. It is obvious that the effect in Chrome is more preferable.

Note: it is necessary to enable experimental web platform features to test it in Chrome.
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — — Splinter Review 
This patch uses a behavior similar to Chrome, which, in my opinion, is more preferable.
Attachment #8489671 - Flags: review?(masayuki)
This patch has passed all tests in Linux x64 opt: https://tbpl.mozilla.org/?tree=Try&rev=fcd273ee4491
Component: Layout → Layout: Text
For users with builds in locales other than Chinese or Japanese, you'll need the testcase to be:

data:text/html;charset=utf-8,<html lang="ja"><p style="-moz-text-align-last: justify;">お早うございます</p>

since we do justification differently depending on content language (see IsJustifiableCharacter in nsTextFrame.cpp, and its aLangIsCJ argument (which callers misname as isCJK)).
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #4)
> For users with builds in locales other than Chinese or Japanese, you'll need
> the testcase to be:
> 
> data:text/html;charset=utf-8,<html lang="ja"><p style="-moz-text-align-last:
> justify;">お早うございます</p>
> 
> since we do justification differently depending on content language (see
> IsJustifiableCharacter in nsTextFrame.cpp, and its aLangIsCJ argument (which
> callers misname as isCJK)).

Oh, I see, if the lang is set to en, there is no longer any space between the characters. Then I have a question that why do we make justification locale-aware? It seems justification in Chrome is not affected by locale.

In the other hand, I guess Korean characters should also be processed like Chinese and Japanese characters as well, since they are similar.
(In reply to Xidorn Quan (UTC+10) from comment #5)
> In the other hand, I guess Korean characters should also be processed like
> Chinese and Japanese characters as well, since they are similar.

Well, that's false. I opened the Korean Wikipedia and found that words are separated by spaces in Korean...
(In reply to Xidorn Quan (UTC+10) from comment #5)
> Then I have a question that why do we make justification
> locale-aware? It seems justification in Chrome is not affected by locale.

because CJ characters may be appear in English or something other languages whose words are separated by spaces. E.g., "good morning is おはよう." In this case, the Japanese characters shouldn't cause additional spaces between each character. They should be displayed as a word in space-separated language's context.
Note that the reason why the first and last character's space are half of the others is, they shouldn't have spaces outside of them (i.e., at the line edges). Therefore, they are ignored from target characters which should be appended extra space. See also bug 288439 comment 16. Note that the bug was discussed with old text API (Gecko 1.8 and earlier).
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #8)
> Note that the reason why the first and last character's space are half of
> the others is, they shouldn't have spaces outside of them (i.e., at the line
> edges). Therefore, they are ignored from target characters which should be
> appended extra space. See also bug 288439 comment 16. Note that the bug was
> discussed with old text API (Gecko 1.8 and earlier).

I still do not understand clearly why those two characters should not have spaces around.

Anyway, I'd like to implement the algorithm described in bug 288439 comment 16 which identifies the points and distributes characters accordingly, so that all the spaces could be equal. If it is necessary to avoid appending space on first and last characters, I'll conform. Does it make sense?
(In reply to Xidorn Quan (UTC+10) from comment #9)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23,
> JST) from comment #8)
> > Note that the reason why the first and last character's space are half of
> > the others is, they shouldn't have spaces outside of them (i.e., at the line
> > edges). Therefore, they are ignored from target characters which should be
> > appended extra space. See also bug 288439 comment 16. Note that the bug was
> > discussed with old text API (Gecko 1.8 and earlier).
> 
> I still do not understand clearly why those two characters should not have
> spaces around.

Not appending extra space for them is a bug. It was a remaining bug at switching new nsTextFrame in Gecko 1.9.

> Anyway, I'd like to implement the algorithm described in bug 288439 comment
> 16 which identifies the points and distributes characters accordingly, so
> that all the spaces could be equal.

Great!

> If it is necessary to avoid appending
> space on first and last characters, I'll conform. Does it make sense?

Yeah, I'll check the behavior when you attach new patch!
Comment on attachment 8489671 [details] [diff] [review]
patch

Waiting next patch.
Attachment #8489671 - Flags: review?(masayuki)
Attached patch patch (obsolete) — — Splinter Review 
The new patch.
Attachment #8489671 - Attachment is obsolete: true
Attachment #8492512 - Flags: review?(masayuki)
Attached file testcases — 
Some justification testcases.
Sorry, I cannot review it soon. Please wait until Wed.
Attached patch patch (obsolete) — — Splinter Review 
Fix a reftest failure and some other cases.

I'm going to work on some tests for the new algorithm.
Attachment #8492512 - Attachment is obsolete: true
Attachment #8492512 - Flags: review?(masayuki)
Attachment #8492582 - Flags: review?(masayuki)
BTW, the latest patch passed all tests in Linux x64 Opt build: https://tbpl.mozilla.org/?tree=Try&rev=eb4dbb93670d
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #14)
> Sorry, I cannot review it soon. Please wait until Wed.

No worry, I'll wait. And I also need time to write some tests.
Attached patch patch for test — — Splinter Review 

        Attachment #8492739 -
        Flags: review?(masayuki)

    
    

    
  
Comment on attachment 8492582 [details] [diff] [review]
patch

You changes the position of nsTextFrame for appending extra spaces before or after the frame. However, nsTextFrame width is very important, so, you shouldn't do that:

1. Select the text, then, you can see non-selected spaces between nsTextFrames.
2. Test underline across two or more nsTextFrames. Your patch causes underline is not painted between nsTextFrames.

FYI:

When you define struct, first |{| should be in next line, i.e.,

struct foo
{
  bar mBazz;
};

> -  mTextJustificationNumSpaces = 0;
> -  mTextJustificationNumLetters = 0;
> +  mTextJustificationNumPoints = 0;
> +  mTextStartJustifiable = false;
> +  mTextEndJustifiable = false;

Like this, when you need to define a lot of variables for same purpose, you should define a simple struct.  Then, the names become simpler and easy to copy and pass to methods.

        Attachment #8492582 -
        Flags: review?(masayuki) → review-

    
    

    
  
Comment on attachment 8492739 [details] [diff] [review]
patch for test

You should test underlines too (specify it to the <p> elements)

And also, if underline is specified only <span>, the underline might not be painted before nor after the nsTextFrame, I guess. We need to check the spec.

        Attachment #8492739 -
        Flags: review?(masayuki) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
I changed the algorithm so that text frames are expanded as expected. Now underline is continuous in a whole line, and there is no non-selected spacing anymore. The tradeoff is that, spans area cannot tightly wrap the text inside. As this matches the current behavior of both Gecko and WebKit, I think it's fine.

        Attachment #8492582 -
        Attachment is obsolete: true

        Attachment #8495249 -
        Flags: review?(masayuki)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Fix a crash when there is any trimmed spaces.

        Attachment #8495249 -
        Attachment is obsolete: true

        Attachment #8495249 -
        Flags: review?(masayuki)

        Attachment #8495606 -
        Flags: review?(masayuki)

    
    

    
  
(In reply to Xidorn Quan (UTC+10) from comment #21)
> Created attachment 8495249 [details] [diff] [review]
> patch
> 
> I changed the algorithm so that text frames are expanded as expected. Now
> underline is continuous in a whole line, and there is no non-selected
> spacing anymore.

Looks almost good to me.

> The tradeoff is that, spans area cannot tightly wrap the
> text inside. As this matches the current behavior of both Gecko and WebKit,
> I think it's fine.

IE too.

However, this doesn't match with letter-spacing examples. I'm not sure what's the best for justificaiton spaces, though.
http://www.w3.org/TR/css3-text/#letter-spacing-property

fantasai, how about you?

On the other hand, at least for now, it's okay to me because the behavior is similar to the other browsers.
Flags: needinfo?(fantasai.bugs)

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #23)
> However, this doesn't match with letter-spacing examples. I'm not sure
> what's the best for justificaiton spaces, though.
> http://www.w3.org/TR/css3-text/#letter-spacing-property

As it is an existing problem that the behavior doesn't match the spec, it might be better to track that in some other bug instead of this.

    
    

    
  
Comment on attachment 8495606 [details] [diff] [review]
patch

>+  const JustificationInfo emptyJustification = {
>+    0, false, false,
>+    false, false, false, false
>+  };
>+  mJustification = emptyJustification;

Why don't you create default constructor which clears all members with 0 or false.

If doing it makes damage to the performance, I think the struct should have Clear() or something similar method.

>@@ -2342,27 +2344,36 @@ nsLineLayout::TrimTrailingWhiteSpaceIn(P
> #ifdef NOISY_TRIM
>       nsFrame::ListTag(stdout, (psd == mRootSpan
>                                 ? mBlockReflowState->frame
>                                 : psd->mFrame->mFrame));
>       printf(": trim of ");
>       nsFrame::ListTag(stdout, pfd->mFrame);
>       printf(" returned %d\n", trimOutput.mDeltaWidth);
> #endif
>-      if (trimOutput.mLastCharIsJustifiable && pfd->mJustificationNumSpaces > 0) {
>-        pfd->mJustificationNumSpaces--;
>-      }
>-      
>+
>       if (trimOutput.mChanged) {
>         pfd->SetFlag(PFD_RECOMPUTEOVERFLOW, true);
>       }
> 
>       if (trimOutput.mDeltaWidth) {
>         pfd->mBounds.ISize(lineWM) -= trimOutput.mDeltaWidth;
> 
>+        if (!pfd->mNext) {
>+          // If any trailing space is trimmed in the last frame in a line,
>+          // the justification point generated by the space should be
>+          // removed as well.
>+          JustificationInfo& justification = pfd->mJustification;
>+          if (justification.mInnerPoints > 0) {
>+            justification.mInnerPoints--;
>+          } else if (justification.mStartJustifiable) {
>+            justification.mStartJustifiable = false;
>+          }
>+        }

Well, I don't understand this well, though. Why do you need to move the code and change the condition?

> void
> nsLineLayout::ComputeJustificationWeights(PerSpanData* aPSD,
>-                                          int32_t* aNumSpaces,
>-                                          int32_t* aNumLetters)
>+                                          int32_t& aInnerPoints)

Why don't you use the result instead of out param? If it is the reason why aPSD is modified too, it's okay. But I guess there could be better method name. InitializeJustificationInfo() or something?

>   for (PerFrameData* pfd = aPSD->mFirstFrame; pfd != nullptr; pfd = pfd->mNext) {
>+    JustificationInfo& justification = pfd->mJustification;
> 
>-    if (true == pfd->GetFlag(PFD_ISTEXTFRAME)) {
>-      numSpaces += pfd->mJustificationNumSpaces;
>-      numLetters += pfd->mJustificationNumLetters;
>+    if (pfd->GetFlag(PFD_ISTEXTFRAME)) {
>+      aInnerPoints += justification.mInnerPoints;
>+    } else if (pfd->mSpan != nullptr) {

} else if (pfd->mSpan) {

>+      PerSpanData* span = pfd->mSpan;
>+      ComputeJustificationWeights(span, aInnerPoints);
>+      justification.mStartJustifiable = span->mFirstFrame &&
>+        span->mFirstFrame->mJustification.mStartJustifiable;
>+      justification.mEndJustifiable = span->mLastFrame &&
>+        span->mLastFrame->mJustification.mEndJustifiable;

How about to add utility method IsStartJustifiable() and IsEndJustificable() in PerSpanData? Then, here will be easier to read.

>+    if (justification.mStartJustifiable && pfd->mPrev) {
>+      justification.mSpaceAtStart = true;
>+      if (pfd->mPrev->mJustification.mEndJustifiable) {
>+        pfd->mPrev->mJustification.mExtraSpaceAtEnd = false;
>+      } else {
>+        justification.mExtraSpaceAtStart = true;
>+        aInnerPoints++;

Hmm, I don't like aInnerPointer is both in/out param. I think that it should be just result and callers increment the result themselves. It's clearer for other developers what this method does.

So, the method could be:

int32_t GetJustificatificationInnerPointsAndInitSpanData(PerSpanData* aPSD)?

> nscoord 
> nsLineLayout::ApplyFrameJustification(PerSpanData* aPSD, FrameJustificationState* aState)
> {
>   NS_ASSERTION(aPSD, "null arg");
>   NS_ASSERTION(aState, "null arg");
> 
>+  if (aPSD->mFrame) {
>+    JustificationInfo& frameJustification = aPSD->mFrame->mJustification;
>+    if (aPSD->mFirstFrame) {
>+      JustificationInfo& justification = aPSD->mFirstFrame->mJustification;

firstFrameJustification? Or JustificationInfo should have AssignStartData(const JustificationInfor&)?

>+      justification.mSpaceAtStart = frameJustification.mSpaceAtStart;
>+      justification.mExtraSpaceAtStart = frameJustification.mExtraSpaceAtStart;
>+    }
>+    if (aPSD->mLastFrame) {
>+      JustificationInfo& justification = aPSD->mLastFrame->mJustification;

lastFrameJustification? Or JustificationInfo should have AssignEndData(const JustificationInfor&)?

>-          nscoord newAllocatedWidthForSpaces =
>-            (aState->mTotalWidthForSpaces*aState->mNumSpacesProcessed)
>-              /aState->mTotalNumSpaces;
>-          
>-          dw += newAllocatedWidthForSpaces - aState->mWidthForSpacesProcessed;
>+          int32_t halves = justification.mInnerPoints * 2;
>+          if (justification.mSpaceAtStart) {
>+            halves++;
>+            flags |= JUSTIFICATION_FLAG_BEFORE;
>+            if (justification.mExtraSpaceAtStart) {
>+              halves++;
>+              flags |= JUSTIFICATION_FLAG_BEFORE_EXTRA;
>+            }
>+          }
>+          if (justification.mSpaceAtEnd) {
>+            halves++;
>+            flags |= JUSTIFICATION_FLAG_AFTER;
>+            if (justification.mExtraSpaceAtEnd) {
>+              halves++;
>+              flags |= JUSTIFICATION_FLAG_AFTER_EXTRA;
>+            }
>+          }

Looks like that this can be two methods of JustificationInfo like GetNumberOfHalfSpaces() and GetJustificationFlags().

>+          static_cast<nsTextFrame*>(pfd->mFrame)->SetJustificationFlags(flags);
> 
>-          aState->mWidthForSpacesProcessed = newAllocatedWidthForSpaces;
>+          aState->mNumPointHalvesProcessed += halves;
>+          nscoord newAllocated =
>+            (aState->mTotalWidthAvailable * aState->mNumPointHalvesProcessed) /
>+            aState->mTotalNumPointHalves;

And also this can be a method like FrameJustificationState::GetAllocatedSpaceWidth()? Anyway, separating computation between struct members helps us to understand what your patch does.

>       else {
>         if (nullptr != pfd->mSpan) {
>-          dw += ApplyFrameJustification(pfd->mSpan, aState);
>+          dw = ApplyFrameJustification(pfd->mSpan, aState);
>         }
>       }

Could you sort out this here?

} else if (pfd->mSpan) {
  dw = ApplyFrameJustification(pfd->mSpan, aState);
}

>-        if (numSpaces > 0) {
>-          FrameJustificationState state =
>-            { numSpaces, numLetters, remainingISize, 0, 0, 0, 0, 0 };
>+        if (innerPoints > 0) {

Hmm, numInsertionPoints might be better...

>+          FrameJustificationState state = {
>+            innerPoints * 2, // mTotalNumPointHalves
>+            remainingISize,  // mTotalWidthAvailable
>+            0,               // mNumPointHalvesProcessed
>+            0,               // mWidthProcessed
>+          };

I think that you should create default constructor or Clear() method for this too.

>+
>+          NS_ASSERTION(
>+              state.mNumPointHalvesProcessed == state.mTotalNumPointHalves,
>+              "Unprocessed justification points");
>+          NS_ASSERTION(state.mWidthProcessed == state.mTotalWidthAvailable,
>+                       "Unprocessed justification width");

If you assume that they never occur, use MOZ_ASSERT(). Otherwise, NS_ASSERTION() is okay.

>@@ -106,19 +107,22 @@ public:
>    * Handle all the relative positioning in the line, compute the
>    * combined area (== overflow area) for the line, and handle view
>    * sizing/positioning and the setting of the overflow rect.
>    */
>   void RelativePositionFrames(nsOverflowAreas& aOverflowAreas);
> 
>   // Support methods for word-wrapping during line reflow
> 
>-  void SetTextJustificationWeights(int32_t aNumSpaces, int32_t aNumLetters) {
>-    mTextJustificationNumSpaces = aNumSpaces;
>-    mTextJustificationNumLetters = aNumLetters;
>+  void SetTextJustificationWeights(int32_t aInnerPoints,
>+                                   bool aStartJustifiable,
>+                                   bool aEndJustifiable) {

nit: |{| should be next line.

>+    mJustification.mInnerPoints = aInnerPoints;
>+    mJustification.mStartJustifiable = aStartJustifiable;
>+    mJustification.mEndJustifiable = aEndJustifiable;

Hmm, it's better if they are an independent struct...

>@@ -341,16 +345,40 @@ protected:
>   
>   // XXX remove this when landing bug 154892 (splitting absolute positioned frames)
>   friend class nsInlineFrame;
> 
>   nsBlockReflowState* mBlockRS;/* XXX hack! */
> 
>   nsLineList::iterator mLineBox;
> 
>+  // The justification process has two stages, which are done by
>+  // ComputeJustificationWeights and ApplyFrameJustification
>+  // respectively.
>+  struct JustificationInfo
>+  {
>+    // The following three fields include the information required
>+    // in the first stage. For text frames, they are filled in
>+    // ReflowFrame, while for other frames, they are filled
>+    // recursively during the first stage.
>+    int32_t mInnerPoints;

mNumInsertionPoints?

>+    bool mStartJustifiable  : 1;

Starting with mIs is clearer for bool members.

>+    bool mEndJustifiable    : 1;
>+    // The following four fields include the information required
>+    // in the second stage. Except m[Extra]SpaceAtStart of first
>+    // frame of each span, and m[Extra]SpaceAtEnd of end frame of
>+    // each span, all of them will be computed in the first stage.
>+    // The exceptions will be figured out in the second stage
>+    // recursively.
>+    bool mSpaceAtStart      : 1;
>+    bool mExtraSpaceAtStart : 1;
>+    bool mSpaceAtEnd        : 1;
>+    bool mExtraSpaceAtEnd   : 1;
>+  };

So, cannot separate first tree members and the others are different stcuts? I feel odd a struct have different stage's data.

>+  struct FrameJustificationState
>+  {
>+    int32_t mTotalNumPointHalves;
>+    nscoord mTotalWidthAvailable;
>+    int32_t mNumPointHalvesProcessed;
>+    nscoord mWidthProcessed;
>   };

Could you explain each members what mean.

>+  void SetTextJustificationWeights(nsLineLayout& aLineLayout) const {

nit: |{| should be next line.

And could you add a comment to explain what this method does.

>+    aLineLayout.SetTextJustificationWeights(
>+        mInnerJustificationPoints, mStartJustifiable, mEndJustifiable);

nit: Indentation in nsLineLayout should be 2 white spaces.

>@@ -2925,41 +2930,30 @@ protected:
>   // How far we've done tab-width calculation; this is ONLY valid when
>   // mTabWidths is nullptr (otherwise rely on mTabWidths->mLimit instead).
>   // It's a DOM offset relative to the current frame's offset.
>   uint32_t              mTabWidthsAnalyzedLimit;
> 
>   int32_t               mLength; // DOM string length, may be INT32_MAX
>   gfxFloat              mWordSpacing;     // space for each whitespace char
>   gfxFloat              mLetterSpacing;   // space for each letter
>-  gfxFloat              mJustificationSpacing;
>   gfxFloat              mHyphenWidth;
>   gfxFloat              mOffsetFromBlockOriginForTabs;
>   bool                  mReflowing;
>   nsTextFrame::TextRunType mWhichTextRun;
>+
>+  gfxFloat              mHalfJustificationSpacing;
>+  uint32_t              mInnerJustificationPoints;
>+  uint32_t              mJustificationBaseStart;
>+  uint32_t              mJustificationBaseEnd;
>+  nsTArray<uint8_t>     mJustificationFlags;
>+  bool                  mStartJustifiable;
>+  bool                  mEndJustifiable;

You should explain each member with comment.

And all bool members should be defined at the end of members including bool mReflowing. In this case, mReflowing wastes the memory space.

Sorry, I don't have much time today to review the TextFrame part.

I hope you should create some utility methods to each struct. If it's possible in TextFrame too. Separating to small code to a method is easier to read since the method name can be self-document. So, you should do that as far as possible.

I'll be back on next Monday for the remaining part.

        Attachment #8495606 -
        Flags: review?(masayuki)

    
    

    
  
FYI: Your patch causes characters may not be center of the space including its glyph and additional space. Therefore, in some cases, it's difficult to select a character by mouse. I think that it's okay to separate this issue to another bug, tough.

    
    

    
  
(In reply to Xidorn Quan (UTC+10) from comment #24)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23,
> JST) from comment #23)
> > However, this doesn't match with letter-spacing examples. I'm not sure
> > what's the best for justificaiton spaces, though.
> > http://www.w3.org/TR/css3-text/#letter-spacing-property
> 
> As it is an existing problem that the behavior doesn't match the spec, it
> might be better to track that in some other bug instead of this.

Yes, of course. But the case of justification should be defined by the spec explicitly.

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #26)
> FYI: Your patch causes characters may not be center of the space including
> its glyph and additional space. Therefore, in some cases, it's difficult to
> select a character by mouse. I think that it's okay to separate this issue
> to another bug, tough.

If you think it is better to make non-justifiable character have spacing around so that most justifiable character could be centered, I could do so as well. But I prefer the current method. Considering the difficulty of selecting characters, moving to the method WebKits uses may help as well. In most case, they append the spacing after characters. But I think that is not preferable as well.

I guess a better way for this problem is to improve the selecting logic instead of justification?

    
    

    
  
(In reply to Xidorn Quan (UTC+10) from comment #28)
> I guess a better way for this problem is to improve the selecting logic
> instead of justification?

I think so but I have no actual idea.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for improving selecting (obsolete)
        — 
          — Splinter Review
      

    


  
The patch to improve selecting. I wonder this patch should be applied independently or with the previous patch.

        Attachment #8496367 -
        Flags: feedback?(masayuki)

    
    

    
  
I'm not sure what's the best behavior, but I imagine it's easiest to implement letter-spacing and justification spacing behaving the same way.
Flags: needinfo?(fantasai.bugs)

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #31)
> FYI: Looks like your patch causes bustage on B2G ICS.
> https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=bc9a9f298448

That's trivial problem. Simply moving the local type out of the function can solve. Or we may ask the build team to investigate using some new version of gcc: the latest android-ndk is using 4.8 instead of 4.4, and both clang and msvc have supported that feature.

    
    

    
  
(In reply to fantasai from comment #32)
> I'm not sure what's the best behavior, but I imagine it's easiest to
> implement letter-spacing and justification spacing behaving the same way.

I agree. Could you mention it in the draft?

# Anyway, we won't do that at least in this bug because our letter-spacing behavior still doesn't conform to the draft's example.
Flags: needinfo?(fantasai.bugs)

    
    

    
  
Comment on attachment 8496367 [details] [diff] [review]
patch for improving selecting

Oh, this is great! Looks and works fine to me!

        Attachment #8496367 -
        Flags: feedback?(masayuki) → feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch simplifies some code by introducing util structures.

        Attachment #8495606 -
        Attachment is obsolete: true

        Attachment #8500975 -
        Flags: review?(masayuki)

    
  

        Attachment #8496367 -
        Flags: review?(ehsan.akhgari)

    
    

    
  
Comment on attachment 8496367 [details] [diff] [review]
patch for improving selecting

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

I'm sorry, I don't know this code very well, and don't feel comfortable reviewing it.

        Attachment #8496367 -
        Flags: review?(ehsan.akhgari)

    
  

        Attachment #8496367 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8500975 [details] [diff] [review]
patch

>+struct JustificationCount
>+{
>+  // Number of insert points inside a given range of clusters. It does
>+  // not include any points before or after the range of clusters.
>+  int32_t mInnerPoints;
>+  // Whether it is possible that there are insert points before or
>+  // after the range of clusters.
>+  bool    mIsStartJustifiable;
>+  bool    mIsEndJustifiable;

nit: Please use only a white space between type and member name because if a member whose type name is too long will be added, this indent doesn't make sense with the new member. If we change indent of all members, it makes hg annotation messy. So, not aligning the member names to same column is better for the future.

>+struct JustificationFlags
>+{
>+  bool mSpaceAtStart      : 1;
>+  bool mExtraSpaceAtStart : 1;
>+  bool mSpaceAtEnd        : 1;
>+  bool mExtraSpaceAtEnd   : 1;

IIRC, bool should be 1 byte. Why do you need to use ": 1" here? If they are not necessary, please remove them.

>+  int32_t SpacesNumberAtStart() const
>+  {
>+    return !mSpaceAtStart ? 0 : (!mExtraSpaceAtStart ? 1 : 2);
>+  }

nit: NumberOfSpacesAtStart() or CountOfSpacesAtStart() ?

>+  int32_t SpacesNumberAtEnd() const
>+  {
>+    return !mSpaceAtEnd ? 0 : (!mExtraSpaceAtEnd ? 1 : 2);
>+  }

ditto.

>+  int32_t SpacesNumber() const
>+  {
>+    return SpacesNumberAtStart() + SpacesNumberAtEnd();
>+  }

ditto.

>+  void CopySpacesAtStart(const JustificationFlags& aCopy)
>+  {
>+    mSpaceAtStart = aCopy.mSpaceAtStart;
>+    mExtraSpaceAtStart = aCopy.mExtraSpaceAtStart;
>+  }

nit: We're usually using aOther like this method.

>+  void CopySpacesAtEnd(const JustificationFlags& aCopy)
>+  {
>+    mSpaceAtEnd = aCopy.mSpaceAtEnd;
>+    mExtraSpaceAtEnd = aCopy.mExtraSpaceAtEnd;
>+  }

ditto.

>+  void SetBothSpacesAtEnd(bool aSet)
>+  {
>+    mSpaceAtEnd = aSet;
>+    mExtraSpaceAtEnd = aSet;
>+  }

"Both" is unclear to me. Perhaps, SetExtraSpaceAtEnd()? And why don't you create for AtStart too?

>+        if (!pfd->mNext) {
>+          // If any trailing space is trimmed in the last frame in a line,
>+          // the justification point generated by the space should be
>+          // removed as well.

Well, I don't understand this comment and following logic... Would you explain by the comment why the struct already includes unnecessary space?

>+          JustificationCount& count = pfd->mJustificationCount;

Although, this is not used in wide range, but justificationCount is better.

>+          if (count.mInnerPoints > 0) {
>+            count.mInnerPoints--;
>+          } else if (count.mIsStartJustifiable) {
>+            count.mIsStartJustifiable = false;
>+          }

Very odd here to me. If the frame is the last frame of a line, here should adjust mIsEndJustificable, isn't it?

>+int32_t
>+nsLineLayout::CountInnerJustifiablePoints(PerSpanData* aPSD)
> {
>   NS_ASSERTION(aPSD, "null arg");
>+  int32_t result = 0;
> 
>   for (PerFrameData* pfd = aPSD->mFirstFrame; pfd != nullptr; pfd = pfd->mNext) {
>+    JustificationCount& count = pfd->mJustificationCount;

nit: justificationCount

>+    if (pfd->GetFlag(PFD_ISTEXTFRAME)) {
>+      result += count.mInnerPoints;
>+    } else if (pfd->mSpan != nullptr) {
>+      PerSpanData* span = pfd->mSpan;
>+      result += CountInnerJustifiablePoints(span);
>+      count.mIsStartJustifiable = span->mFirstFrame &&
>+        span->mFirstFrame->IsStartJustifiable();

nit:

justificableCount.mIsStartJustifiable =
  span->mFirstFrame && span->mFirstFrame->IsStartJustifiable();

is better if it's less than 80 characters.

>+      count.mIsEndJustifiable = span->mLastFrame &&
>+        span->mLastFrame->IsEndJustifiable();

ditto.

>+    }
> 
>+    JustificationFlags& flags = pfd->mJustificationFlags;

nit: justificationFlags

>+    flags = JustificationFlags();
>+    if (count.mIsStartJustifiable && pfd->mPrev) {

>+      flags.mSpaceAtStart = true;
>+      if (pfd->mPrev->IsEndJustifiable()) {
>+        pfd->mPrev->mJustificationFlags.mExtraSpaceAtEnd = false;
>+      } else {
>+        flags.mExtraSpaceAtStart = true;
>+        result++;

Sounds odd. CountInnerJustifiablePoints() includes the space of each side??

>+      }

How about move this (except result++) to JustificationFlags::SetSpaceAtStart(JustificationFlags& aPreviousJustificationFlags)?

>     }
>+    if (count.mIsEndJustifiable && pfd->mNext) {
>+      flags.mSpaceAtEnd = true;
>+      flags.mExtraSpaceAtEnd = true;

Setting extra space flag here feels odd to me. It's necessary only when following frame doesn't have justificatable character at first of the frame. So, I think that false is normal case for extra space. If you agree with this and it doesn't make the code complicated, would you do so? (I guess you need to change only above block).

> nscoord 
> nsLineLayout::ApplyFrameJustification(PerSpanData* aPSD, FrameJustificationState* aState)
> {
>   NS_ASSERTION(aPSD, "null arg");
>   NS_ASSERTION(aState, "null arg");
> 
>   nscoord deltaICoord = 0;
>   for (PerFrameData* pfd = aPSD->mFirstFrame; pfd != nullptr; pfd = pfd->mNext) {
>     // Don't reposition bullets (and other frames that occur out of X-order?)
>     if (!pfd->GetFlag(PFD_ISBULLET)) {
>       nscoord dw = 0;
>       WritingMode lineWM = mRootSpan->mWritingMode;
>+      JustificationFlags flags = pfd->mJustificationFlags;

nit: justificationFlags

>+          aState->mNumPointHalvesProcessed +=
>+            pfd->mJustificationCount.mInnerPoints * 2 + flags.SpacesNumber();

How about to create JustificationCount::CountOfSpaces(const JustificationFlags& aJustificationFlags)?

>+      } else if (pfd->mSpan && pfd->mSpan->mFirstFrame) {
>+        PerSpanData* span = pfd->mSpan;
>+        span->mFirstFrame->mJustificationFlags.CopySpacesAtStart(flags);
>+        span->mLastFrame->mJustificationFlags.CopySpacesAtEnd(flags);
>+        dw = ApplyFrameJustification(span, aState);
>       }
> 
>       pfd->mBounds.ISize(lineWM) += dw;
> 
>       deltaICoord += dw;
>       pfd->mFrame->SetRect(lineWM, pfd->mBounds, mContainerWidth);
>     }
>   }

>@@ -2545,34 +2548,43 @@ nsLineLayout::TextAlignLine(nsLineBox* a
>+      case NS_STYLE_TEXT_ALIGN_JUSTIFY: {
>+        int32_t innerPoints = CountInnerJustifiablePoints(psd);

So, this variable name must be wrong. It may includes space at start and/or end.

>+        if (innerPoints > 0) {
>+          FrameJustificationState state = {
>+            innerPoints * 2, // mTotalNumPointHalves
>+            remainingISize,  // mTotalWidthAvailable
>+            0,               // mNumPointHalvesProcessed
>+            0,               // mWidthProcessed
>+          };
> 
>           // Apply the justification, and make sure to update our linebox
>           // width to account for it.
>           aLine->ExpandBy(ApplyFrameJustification(psd, &state),
>                           mContainerWidth);
>+
>+          printf("point halves: %d, processed: %d\n",
>+                 state.mTotalNumPointHalves, state.mNumPointHalvesProcessed);

Please don't include printf() at requesting review.

>+          NS_ASSERTION(
>+              state.mNumPointHalvesProcessed == state.mTotalNumPointHalves,
>+              "Unprocessed justification points");
>+          NS_ASSERTION(state.mWidthProcessed == state.mTotalWidthAvailable,
>+                       "Unprocessed justification width");

If you want to make debug build crashed, you can use MOZ_ASSERT(). Then, you can detect regressions without reftests only on debug build.

>diff --git a/layout/generic/nsLineLayout.h b/layout/generic/nsLineLayout.h
>+  void SetJustificationCount(mozilla::JustificationCount aCount)

|const mozilla::JustificationCount&|?

>+  {
>+    mJustificationCount = aCount;
>   }

>+  struct FrameJustificationState
>+  {
>+    int32_t mTotalNumPointHalves;

How about mCountOfHalfSpaces? "Point" in layout sounds like a point represented by x and y.

>+    nscoord mTotalWidthAvailable;

How about mAvaialbleWidth? "Total" sounds redundant to me.

>+    int32_t mNumPointHalvesProcessed;

How about mCountOfHalfSpacesProcessed?

>+    nscoord mWidthProcessed;
>   };


>diff --git a/layout/generic/nsTextFrame.cpp b/layout/generic/nsTextFrame.cpp
>--- a/layout/generic/nsTextFrame.cpp
>+++ b/layout/generic/nsTextFrame.cpp
>@@ -2932,41 +2931,29 @@ protected:
>+  gfxFloat              mHalfJustificationSpacing;
>+  JustificationCount    mJustificationCount;
>+  uint32_t              mJustificationBaseStart;
>+  uint32_t              mJustificationBaseEnd;

What do "Base"s mean?

Anyway, would you explain each member with comments?

>+  nsTArray<JustificationFlags> mJustificationFlags;

Hmm, I worry about memory fragmentation caused by this. I'll check the users of this.

I'll keep reviewing this after lunch.

    
    

    
  
Comment on attachment 8500975 [details] [diff] [review]
patch

>+  nsTArray<JustificationFlags> mJustificationFlags;

The type name is JustificationFlag"s". So, the member name should be mJustificationFlagsArray.

>+void
>+PropertyProvider::ComputeJustification(int32_t aOffset, int32_t aLength)
>+{
>+  bool isCJ = IsChineseOrJapanese(mFrame);
>+  nsSkipCharsRunIterator
>+    run(mStart, nsSkipCharsRunIterator::LENGTH_INCLUDES_SKIPPED, aLength);
>+  run.SetOriginalOffset(aOffset);
>+  mJustificationBaseStart = run.GetSkippedOffset();
>+
>+  mJustificationFlags.ClearAndRetainStorage();
>+  while (run.NextRun()) {
>+    uint32_t originalOffset = run.GetOriginalOffset();
>+    uint32_t skippedOffset = run.GetSkippedOffset();
>+    uint32_t length = run.GetRunLength();
>+    mJustificationFlags.SetLength(
>+        skippedOffset + length - mJustificationBaseStart);

Wow, this looks reallocating a lot of time! Cannot you guess enough length out of while loop? And if you use nsAutoTArray for it, you can allocate array in the stack at constructing the PropertyProvider. When it's not enough and needs to reallocate, it uses the heap.

>+    gfxSkipCharsIterator iter = run.GetPos();
>+    for (uint32_t i = 0; i < length; ++i) {
>+      uint32_t offset = originalOffset + i;
>+      if (IsJustifiableCharacter(mFrag, offset, isCJ)) {

if (!IsJustifiableCharacter(...)) {
  continue;
}

reduces an indent level.

>+        iter.SetOriginalOffset(offset);
>+
>+        FindClusterStart(mTextRun, originalOffset, &iter);
>+        uint32_t firstChar = iter.GetSkippedOffset() - mJustificationBaseStart;

Could you test if |iter.GetSkippedOffset() - mJustificationBaseStart| is negative? I think that if it were so, should be crashed safely because it might cause random crash which is hard to find the cause. Use MOZ_RELEASE_ASSERT() for that. I.e., MOZ_RELEASE_ASSERT(iter.GetSkippedOffset() >= mJustificationBaseStart, "Something message here");

>+        if (firstChar == 0) {
>+          mJustificationCount.mIsStartJustifiable = true;
>+        } else {
>+          mJustificationFlags[firstChar].mSpaceAtStart = true;
>+          if (mJustificationFlags[firstChar - 1].mSpaceAtEnd) {
>+            mJustificationFlags[firstChar - 1].mExtraSpaceAtEnd = false;
>+          } else {
>+            mJustificationCount.mInnerPoints++;
>+            mJustificationFlags[firstChar].mExtraSpaceAtStart = true;
>+          }
>+        }

Hmm, this is difficult to understand. Could you explain what each case means by comments?

>+        FindClusterEnd(mTextRun, originalOffset + length, &iter);
>+        uint32_t lastChar = iter.GetSkippedOffset() - mJustificationBaseStart;
>+        mJustificationCount.mInnerPoints++;
>+        mJustificationFlags[lastChar].SetBothSpacesAtEnd(true);

ditto. And I also feel odd to set extra space to the end here.

>+
>+        // Skip the whole cluster
>+        i = iter.GetOriginalOffset() - originalOffset;
>+      }
>+    }
>+  }
>+  mJustificationBaseEnd =
>+    mJustificationBaseStart + mJustificationFlags.Length();
>+
>+  if (mJustificationBaseEnd > mJustificationBaseStart &&

|!mJustificationFlags.IsEmpty()|? Then, mJustificationBaseEnd is really necessary yet? Looks like it's enough to be a method.

>+      mJustificationFlags.LastElement().mSpaceAtEnd) {
>+    mJustificationCount.mInnerPoints--;

I feel odd. Why mInnerPoints already includes the space at the end as an inner space?

>+    mJustificationCount.mIsEndJustifiable = true;
>+    mJustificationFlags.LastElement().SetBothSpacesAtEnd(false);

I don't like setting odd value and corrects it later except when it's the only possible way or it's better for performance.

>@@ -3070,45 +3115,28 @@ PropertyProvider::GetSpacingInternal(uin
>   // Now add in justification spacing
>+  if (mHalfJustificationSpacing) {
>+    // If there is any spaces trimmed at the end, aStart + aLength may
>+    // be larger than the flags array. When that happens, we can simply
>+    // ignore those spaces.
>+    auto end = std::min(aStart + aLength, mJustificationBaseEnd);
>+    for (auto i = aStart; i < end; i++) {

I'm not sure if using "auto" is allowed...

>+      const JustificationFlags flags =

const JustificationFlags&?

>+        mJustificationFlags[i - mJustificationBaseStart];
>+      aSpacing[i - aStart].mBefore +=
>+        mHalfJustificationSpacing * flags.SpacesNumberAtStart();
>+      aSpacing[i - aStart].mAfter +=
>+        mHalfJustificationSpacing * flags.SpacesNumberAtEnd();
>     }
>   }

Looks like that you make this method stateful. mJustification* depends on the argument of previous call of ComputeJustification().

Even if it's okay, if the range of this argument includes out of range from the range of ComputJustification(), you should do something. MOZ_ASSERT()?

I'm not sure if this is okay. We should ask additional review to roc later, anyway.

>@@ -8326,30 +8326,23 @@ nsTextFrame::ReflowText(nsLineLayout& aL

>-  // Compute space and letter counts for justification, if required

Why do you remove this comment?

>   if (!textStyle->WhiteSpaceIsSignificant() &&
>       (lineContainer->StyleText()->mTextAlign == NS_STYLE_TEXT_ALIGN_JUSTIFY ||
>        lineContainer->StyleText()->mTextAlignLast == NS_STYLE_TEXT_ALIGN_JUSTIFY) &&
>       !lineContainer->IsSVGText()) {

>+  void SetJustificationFlags(mozilla::JustificationFlags aFlags)
>+  {
>+    mJustificationFlags = aFlags;
>+  }

The argument should be: const mozilla::JustificationFlags& aJustificationFlags

>+  mozilla::JustificationFlags GetJustificationFlags() const
>+  {
>+    return mJustificationFlags;
>+  }

How about:

const mozilla::JustificationFlags& JustificationFlags() const
{
  return mJustificationFlags;
}

I think that the caller shouldn't change the value and "Get" isn't necessary if this never return nullptr.

>+  mozilla::JustificationFlags mJustificationFlags;

I'm afraid you are adding a member to nsTextFrame because nsTextFrame is created tons of times. Although, this might be necessary and enough small... But you should retry to think that if we can avoid to add this to nsTextFrame.

Could you add a warning comment in JustificationUtils.h. Something like: "Be aware! JustificationFlags is a member of nsTextFrame. Therefore, increasing size of this struct may make damage to the footprint.".

But of course, this should be check by roc too.



I still don't understand your patch completely. So, if my review is wrong, let me know.

However, I worry about some points:

1. You use "point" in the patches, but "point" is not good word in layout because there is another "point" which is represented with x and y.
2. You add some justification data as members of PropertyProvider and nsTextFrame. The former makes the class stateful. And The latter increases the memory. Therefore, our current implementation uses hacky way to compute and set the information to nsTextFrame. (Like TrimTrailingWhiteSpace()) If you can avoid to add a member to nsTextFrame, please try it.
3. Justification code is very complicated. Therefore, even we have already touched it, we need to recall it. So, explaining each code what does is very helpful for everyone. Please add comments or create and use utility methods a lot.

    
    

    
  
And I guess that when you write such big patch, you write a patch step by step for each purpose. If so, separating each step to a smaller patch makes review easier and you might find bugs.

For example, you might need to redesign current code before changing the logic. Then, the work should be separated to some patches and actually fixing this bug part should be other patches. If you do so, reviews can focus if changing logic is intentional or not.

    
    

    
  
See bug 960871 for an example of separating patches.

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #39)
> >+        mJustificationFlags[i - mJustificationBaseStart];
> >+      aSpacing[i - aStart].mBefore +=
> >+        mHalfJustificationSpacing * flags.SpacesNumberAtStart();
> >+      aSpacing[i - aStart].mAfter +=
> >+        mHalfJustificationSpacing * flags.SpacesNumberAtEnd();
> >     }
> >   }
> 
> Looks like that you make this method stateful. mJustification* depends on
> the argument of previous call of ComputeJustification().

This class has been stateful before: GetSpacingInternal depends on mJustificationSpacing which is set in SetupJustificationSpacing. Although the former did not depend on ComputeJustification before, I think it's fine since ComputeJustification is only called in one place outside the class. Maybe it is better to rename ComputeJustification to make it clear that it affects the state of the instance?

> >+  void SetJustificationFlags(mozilla::JustificationFlags aFlags)
> >+  {
> >+    mJustificationFlags = aFlags;
> >+  }
> 
> The argument should be: const mozilla::JustificationFlags&
> aJustificationFlags
> 
> >+  mozilla::JustificationFlags GetJustificationFlags() const
> >+  {
> >+    return mJustificationFlags;
> >+  }
> 
> How about:
> 
> const mozilla::JustificationFlags& JustificationFlags() const
> {
>   return mJustificationFlags;
> }

Since JustificationFlags is only 1 byte in my patch (and would be 4 byte if you insist that the flags should be plain bool type instead of bit fields), it is more efficient to transfer it by value instead of reference.

> I think that the caller shouldn't change the value and "Get" isn't necessary
> if this never return nullptr.
> 
> >+  mozilla::JustificationFlags mJustificationFlags;
> 
> I'm afraid you are adding a member to nsTextFrame because nsTextFrame is
> created tons of times. Although, this might be necessary and enough small...
> But you should retry to think that if we can avoid to add this to
> nsTextFrame.

That's why I make all bools an one-bit field, so that the whole flags could be only 1 byte. It is possible to remove the member: we can use 4 state bits instead. Do you think it is a better idea?

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #38)
> >+struct JustificationFlags
> >+{
> >+  bool mSpaceAtStart      : 1;
> >+  bool mExtraSpaceAtStart : 1;
> >+  bool mSpaceAtEnd        : 1;
> >+  bool mExtraSpaceAtEnd   : 1;
> 
> IIRC, bool should be 1 byte. Why do you need to use ": 1" here? If they are
> not necessary, please remove them.

It is useful to reduce the size of the structure. By making them bit fields, the whole structure uses only 1 byte instead of 4 bytes.

> >+          JustificationCount& count = pfd->mJustificationCount;
> 
> Although, this is not used in wide range, but justificationCount is better.
> 
> >+          if (count.mInnerPoints > 0) {
> >+            count.mInnerPoints--;
> >+          } else if (count.mIsStartJustifiable) {
> >+            count.mIsStartJustifiable = false;
> >+          }
> 
> Very odd here to me. If the frame is the last frame of a line, here should
> adjust mIsEndJustificable, isn't it?

In this case, there is no inner points (count.mInnerPoints == 0) which means the whole frame contains only a single space which is trimmed. We don't want to count any justification point before this frame because of the trimmed space, hence mIsStartJustifiable is cleared. Since it is at the end of a line, and mIsEndJustifiable is never made its effect, it is not necessary to clear it.

> >@@ -2545,34 +2548,43 @@ nsLineLayout::TextAlignLine(nsLineBox* a
> >+      case NS_STYLE_TEXT_ALIGN_JUSTIFY: {
> >+        int32_t innerPoints = CountInnerJustifiablePoints(psd);
> 
> So, this variable name must be wrong. It may includes space at start and/or
> end.

It's not wrong. It includes inner points in the context of the given span. It does not include any space before the start or after the end of the span.

> 
> >+        if (innerPoints > 0) {
> >+          FrameJustificationState state = {
> >+            innerPoints * 2, // mTotalNumPointHalves
> >+            remainingISize,  // mTotalWidthAvailable
> >+            0,               // mNumPointHalvesProcessed
> >+            0,               // mWidthProcessed
> >+          };
> > 
> >           // Apply the justification, and make sure to update our linebox
> >           // width to account for it.
> >           aLine->ExpandBy(ApplyFrameJustification(psd, &state),
> >                           mContainerWidth);
> >+
> >+          printf("point halves: %d, processed: %d\n",
> >+                 state.mTotalNumPointHalves, state.mNumPointHalvesProcessed);
> 
> Please don't include printf() at requesting review.

Sorry, I forgot to remove it before submitting.

> >diff --git a/layout/generic/nsLineLayout.h b/layout/generic/nsLineLayout.h
> >+  void SetJustificationCount(mozilla::JustificationCount aCount)
> 
> |const mozilla::JustificationCount&|?
> 
> >+  {
> >+    mJustificationCount = aCount;
> >   }
> 
> >+  struct FrameJustificationState
> >+  {
> >+    int32_t mTotalNumPointHalves;
> 
> How about mCountOfHalfSpaces? "Point" in layout sounds like a point
> represented by x and y.

But "Space" sounds like a space character to me. Maybe it is better to call it justifiable position, hence "Pos" here?

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #40)
> And I guess that when you write such big patch, you write a patch step by
> step for each purpose. If so, separating each step to a smaller patch makes
> review easier and you might find bugs.

I think I write the patch by its whole, and I found it hard to separate it into smaller patches since they are coupling tightly. The logic is completely different. The former method bases on justifiable characters, but in this patch it bases on justifiable positions, which makes every place different. Hence it is hard to change some places in one patch and others in another.

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC-7) from comment #42)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #39)
> > >+        mJustificationFlags[i - mJustificationBaseStart];
> > >+      aSpacing[i - aStart].mBefore +=
> > >+        mHalfJustificationSpacing * flags.SpacesNumberAtStart();
> > >+      aSpacing[i - aStart].mAfter +=
> > >+        mHalfJustificationSpacing * flags.SpacesNumberAtEnd();
> > >     }
> > >   }
> > 
> > Looks like that you make this method stateful. mJustification* depends on
> > the argument of previous call of ComputeJustification().
> 
> This class has been stateful before: GetSpacingInternal depends on
> mJustificationSpacing which is set in SetupJustificationSpacing. Although
> the former did not depend on ComputeJustification before, I think it's fine
> since ComputeJustification is only called in one place outside the class.

Okay.

> Maybe it is better to rename ComputeJustification to make it clear that it
> affects the state of the instance?

Yeah, I think so. And should be checked with MOZ_ASSERT().

> > const mozilla::JustificationFlags& JustificationFlags() const
> > {
> >   return mJustificationFlags;
> > }
> 
> Since JustificationFlags is only 1 byte in my patch (and would be 4 byte if
> you insist that the flags should be plain bool type instead of bit fields),
> it is more efficient to transfer it by value instead of reference.

I don't think that copying 4 byte is slower than 1 byte, but not 100% sure. Either is okay but if you don't use reference, I'd like you to add check the size with static_assert() at build time.

> > I think that the caller shouldn't change the value and "Get" isn't necessary
> > if this never return nullptr.
> > 
> > >+  mozilla::JustificationFlags mJustificationFlags;
> > 
> > I'm afraid you are adding a member to nsTextFrame because nsTextFrame is
> > created tons of times. Although, this might be necessary and enough small...
> > But you should retry to think that if we can avoid to add this to
> > nsTextFrame.
> 
> That's why I make all bools an one-bit field, so that the whole flags could
> be only 1 byte. It is possible to remove the member: we can use 4 state bits
> instead. Do you think it is a better idea?

I think that using bit field is better as you think. However, when it's used as a member of a class, it might use 4 or 8 byte (32bit OS / 64bit OS). (If there are some bool members around it, it could be merged, though)

So, if you can remove it, it's the best.


(In reply to Xidorn Quan [:xidorn] (UTC-7) from comment #43)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #38)
> > >+struct JustificationFlags
> > >+{
> > >+  bool mSpaceAtStart      : 1;
> > >+  bool mExtraSpaceAtStart : 1;
> > >+  bool mSpaceAtEnd        : 1;
> > >+  bool mExtraSpaceAtEnd   : 1;
> > 
> > IIRC, bool should be 1 byte. Why do you need to use ": 1" here? If they are
> > not necessary, please remove them.
> 
> It is useful to reduce the size of the structure. By making them bit fields,
> the whole structure uses only 1 byte instead of 4 bytes.

Ah, okay. I misunderstood. Keep going.

> > How about mCountOfHalfSpaces? "Point" in layout sounds like a point
> > represented by x and y.
> 
> But "Space" sounds like a space character to me. Maybe it is better to call
> it justifiable position, hence "Pos" here?

Hmm, I don't like "Pos" too...

In the spec, the points which are inserted spaces between characters are called, "expansion opportunity".
http://www.w3.org/TR/css3-text/#expanding-text

But it's too long and probably there is only one expansion opportunity between two justifiable characters...

Instead, can we call of each side of a justifiable character as "expandable sides" or "justifiable sides"?

Then, could be as:

struct FrameJustificationState
{
  struct
  {
    int32_t mCount;
    int32_t mHandled;
  } mExpandableSides;
  struct
  {
    nscoord mAvailable;
    nscoord mConsumed;
  } mExpandableWidth;
}

?

(In reply to Xidorn Quan [:xidorn] (UTC-7) from comment #44)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #40)
> > And I guess that when you write such big patch, you write a patch step by
> > step for each purpose. If so, separating each step to a smaller patch makes
> > review easier and you might find bugs.
> 
> I think I write the patch by its whole, and I found it hard to separate it
> into smaller patches since they are coupling tightly.

Yeah, I can imagine...

> The logic is completely different. The former method bases on justifiable characters, but
> in this patch it bases on justifiable positions, which makes every place
> different. Hence it is hard to change some places in one patch and others in
> another.

Looks like that your patch changes handling flow too. If this is correct, can you separate the flow change part and logic change part? But anyway, it's optional.

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC-7) from comment #43)
> > >+          if (count.mInnerPoints > 0) {
> > >+            count.mInnerPoints--;
> > >+          } else if (count.mIsStartJustifiable) {
> > >+            count.mIsStartJustifiable = false;
> > >+          }
> > 
> > Very odd here to me. If the frame is the last frame of a line, here should
> > adjust mIsEndJustificable, isn't it?
> 
> In this case, there is no inner points (count.mInnerPoints == 0) which means
> the whole frame contains only a single space which is trimmed.

For example, a text frame "abc" also has 0 inner points, isn't it?

I understand the value of JustificationCount::mInnterPoints for "abc 文字" is:

a   b   c ' '  文  字
          ^ ^ ^ ^ ^

Then, 5. Or if "abc文字" then,

a   b   c   文  字
         ^ ^ ^ ^
the value must be 4. Is this correct?

Therefore, I don't understand why you decrease mInnerPoints from the last frame.

Additionally, "文<span>字</span>" is there, I guess that both mInnerPoints values are 0 and mIsEndJustifiable of the former is true, mIsStartJustifiable of the latter is true. Therefore, I don't understand the second case (clearing mIsStartJustifiable).

> > >@@ -2545,34 +2548,43 @@ nsLineLayout::TextAlignLine(nsLineBox* a
> > >+      case NS_STYLE_TEXT_ALIGN_JUSTIFY: {
> > >+        int32_t innerPoints = CountInnerJustifiablePoints(psd);
> > 
> > So, this variable name must be wrong. It may includes space at start and/or
> > end.
> 
> It's not wrong. It includes inner points in the context of the given span.
> It does not include any space before the start or after the end of the span.

Ah, understood. You're right.

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #45)
> > But "Space" sounds like a space character to me. Maybe it is better to call
> > it justifiable position, hence "Pos" here?
> 
> Hmm, I don't like "Pos" too...
> 
> In the spec, the points which are inserted spaces between characters are
> called, "expansion opportunity".
> http://www.w3.org/TR/css3-text/#expanding-text
> 
> But it's too long and probably there is only one expansion opportunity
> between two justifiable characters...

Yes, the opportunity seems to be the exact expression. And there is another phase called "break opportunity" which in our current code is called "OptionalBreakPosition". That's why I proposed "Pos". So maybe "oppty"/"oppties" with some comments here?

> Instead, can we call of each side of a justifiable character as "expandable
> sides" or "justifiable sides"?
> 
> Then, could be as:
> 
> struct FrameJustificationState
> {
>   struct
>   {
>     int32_t mCount;
>     int32_t mHandled;
>   } mExpandableSides;
>   struct
>   {
>     nscoord mAvailable;
>     nscoord mConsumed;
>   } mExpandableWidth;
> }
> 
> ?

This looks good to me. So we can replace "halves" with "sides". But what do you think about "points"?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #46)
> (In reply to Xidorn Quan [:xidorn] (UTC-7) from comment #43)
> > > >+          if (count.mInnerPoints > 0) {
> > > >+            count.mInnerPoints--;
> > > >+          } else if (count.mIsStartJustifiable) {
> > > >+            count.mIsStartJustifiable = false;
> > > >+          }
> > > 
> > > Very odd here to me. If the frame is the last frame of a line, here should
> > > adjust mIsEndJustificable, isn't it?
> > 
> > In this case, there is no inner points (count.mInnerPoints == 0) which means
> > the whole frame contains only a single space which is trimmed.
> 
> For example, a text frame "abc" also has 0 inner points, isn't it?
> 
> I understand the value of JustificationCount::mInnterPoints for "abc 文字" is:
> 
> a   b   c ' '  文  字
>           ^ ^ ^ ^ ^

Hmmm... I think it's 3:

a b c ' ' 文 字
     ^   ^  ^

> Then, 5. Or if "abc文字" then,
> 
> a   b   c   文  字
>          ^ ^ ^ ^
> the value must be 4. Is this correct?

It's 2.

> Therefore, I don't understand why you decrease mInnerPoints from the last
> frame.
> 
> Additionally, "文<span>字</span>" is there, I guess that both mInnerPoints
> values are 0 and mIsEndJustifiable of the former is true,
> mIsStartJustifiable of the latter is true. Therefore, I don't understand the
> second case (clearing mIsStartJustifiable).

In your case, there is no space be trimmed, hence the condition "trimOutput.mDeltaWidth" is not true.

When trimOutput.mDeltaWidth is not zero, the last frame should contains a tailing space which has been trimmed. That frame could either have other characters (so that inner points > 0), or only the space itself (so that mIsStartJustifiable == mIsEndJustifiable == true). In either case, we need to remove the effect of the space because it is trimmed.

This, in the current code, happens to be done via suppressing the justification of the last character in a line. However in my new code, the justification is no longer suppressed, thus it is necessary to remove the justification opportunity introducing by the trimmed space in this way.

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC-7) from comment #47)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #45)
> > > But "Space" sounds like a space character to me. Maybe it is better to call
> > > it justifiable position, hence "Pos" here?
> > 
> > Hmm, I don't like "Pos" too...
> > 
> > In the spec, the points which are inserted spaces between characters are
> > called, "expansion opportunity".
> > http://www.w3.org/TR/css3-text/#expanding-text
> > 
> > But it's too long and probably there is only one expansion opportunity
> > between two justifiable characters...
> 
> Yes, the opportunity seems to be the exact expression. And there is another
> phase called "break opportunity" which in our current code is called
> "OptionalBreakPosition". That's why I proposed "Pos". So maybe
> "oppty"/"oppties" with some comments here?

No, if we'll use "opportunity", it should be abbreviated.

> > struct FrameJustificationState
> > {
> >   struct
> >   {
> >     int32_t mCount;
> >     int32_t mHandled;
> >   } mExpandableSides;
> >   struct
> >   {
> >     nscoord mAvailable;
> >     nscoord mConsumed;
> >   } mExpandableWidth;
> > }
> > 
> > ?
> 
> This looks good to me. So we can replace "halves" with "sides". But what do
> you think about "points"?

I don't like "points". So, I like "sides" better. Although, "sides" are used in CSS border rendering code.

> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #46)
> > (In reply to Xidorn Quan [:xidorn] (UTC-7) from comment #43)
> > > > >+          if (count.mInnerPoints > 0) {
> > > > >+            count.mInnerPoints--;
> > > > >+          } else if (count.mIsStartJustifiable) {
> > > > >+            count.mIsStartJustifiable = false;
> > > > >+          }
> > > > 
> > > > Very odd here to me. If the frame is the last frame of a line, here should
> > > > adjust mIsEndJustificable, isn't it?
> > > 
> > > In this case, there is no inner points (count.mInnerPoints == 0) which means
> > > the whole frame contains only a single space which is trimmed.
> > 
> > For example, a text frame "abc" also has 0 inner points, isn't it?
> > 
> > I understand the value of JustificationCount::mInnterPoints for "abc 文字" is:
> > 
> > a   b   c ' '  文  字
> >           ^ ^ ^ ^ ^
> 
> Hmmm... I think it's 3:
> 
> a b c ' ' 文 字
>      ^   ^  ^
> 
> > Then, 5. Or if "abc文字" then,
> > 
> > a   b   c   文  字
> >          ^ ^ ^ ^
> > the value must be 4. Is this correct?
> 
> It's 2.

Oh, okay. Then, mInnerPoints could be mInnerOpportunities?

Anyway, I'll retry to understand your patch with this information.

> > Therefore, I don't understand why you decrease mInnerPoints from the last
> > frame.
> > 
> > Additionally, "文<span>字</span>" is there, I guess that both mInnerPoints
> > values are 0 and mIsEndJustifiable of the former is true,
> > mIsStartJustifiable of the latter is true. Therefore, I don't understand the
> > second case (clearing mIsStartJustifiable).
> 
> In your case, there is no space be trimmed, hence the condition
> "trimOutput.mDeltaWidth" is not true.

Oh, I see.

> When trimOutput.mDeltaWidth is not zero, the last frame should contains a
> tailing space which has been trimmed. That frame could either have other
> characters (so that inner points > 0), or only the space itself (so that
> mIsStartJustifiable == mIsEndJustifiable == true). In either case, we need
> to remove the effect of the space because it is trimmed.

Thanks for your explanation. I understood.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8500975 -
        Attachment is obsolete: true

        Attachment #8500975 -
        Flags: review?(masayuki)

        Attachment #8502299 -
        Flags: review?(masayuki)

    
    

    
  
Comment on attachment 8496367 [details] [diff] [review]
patch for improving selecting

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

::: gfx/thebes/gfxTextRun.h
@@ +269,5 @@
>       * Uses GetSpacing from aBreakProvider.
>       */
>      gfxFloat GetAdvanceWidth(uint32_t aStart, uint32_t aLength,
> +                             PropertyProvider *aProvider,
> +                             PropertyProvider::Spacing* aSpacing = nullptr);

Document what is returned in aSpacing.

        Attachment #8496367 -
        Flags: review?(roc) → review+

    
    

    
  
Comment on attachment 8502299 [details] [diff] [review]
patch

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

Actually, just request review from me once Masayuki is done :-)

        Attachment #8502299 -
        Flags: review?(roc)

    
    

    
  


  
I'm not very sure about my wording on the comment. Do you think that is ok?

        Attachment #8496367 -
        Attachment is obsolete: true

        Attachment #8502831 -
        Flags: review+
Flags: needinfo?(roc)

    
    

    
  
Comment on attachment 8502831 [details] [diff] [review]
patch 2 - improve selecting, r=roc

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

That's fine, thanks!!

    
    

    
  
Comment on attachment 8502299 [details] [diff] [review]
patch

nit: Could you create a class JustificationUtils which has only static methods you defined/implemented below. (Enough to implement them in the class definition) Then, you can remove "Justification" from their names.

>+// The two functions below are the common part of processing
>+// justification flags of two adjacent parts.
>+
>+static inline void
>+SetJustificationSpaceAtStart(JustificationFlags& aFlags,
>+                             JustificationFlags& aPrevFlags,
>+                             int32_t& aCount)

How about to use result (bool or additional count of spaces?) instead of aCount? If you think that it's harder to understand at reading the callers, keep this style.

>+static inline void
>+SetJustificationSpaceAtEnd(JustificationFlags& aFlags, int32_t& aCount)
>+{
>+  // There is an expansion opportunity after the given part.
>+  aCount++;

And also here.

>+static inline int32_t
>+CountExpansionSpaces(const JustificationCount& aCount,
>+                     const JustificationFlags& aFlags)
>+{
>+  return aCount.mInnerOpportunities * 2 + aFlags.CountOfSpaces();
>+}

I think that you should explain what the result means with comment.

>+// -- Jusitification bits -----------------------------------------------------
>+
>+// These state bits are used to inform the text frame how it should add spaces
>+// at its sides for justification.
>+// See nsLineLayout::ApplyFrameJustification and JustificationUtils.h.
>+FRAME_STATE_BIT(Text, 33, TEXT_JUSTIFICATION_SPACE_AT_START)
>+FRAME_STATE_BIT(Text, 34, TEXT_JUSTIFICATION_EXTRA_SPACE_AT_START)
>+FRAME_STATE_BIT(Text, 35, TEXT_JUSTIFICATION_SPACE_AT_END)
>+FRAME_STATE_BIT(Text, 36, TEXT_JUSTIFICATION_EXTRA_SPACE_AT_END)

Hmm, I cannot review this. Roc or dbaron needs to check when you add new flags.

>@@ -2342,27 +2340,36 @@ nsLineLayout::TrimTrailingWhiteSpaceIn(P
> #ifdef NOISY_TRIM
>       nsFrame::ListTag(stdout, (psd == mRootSpan
>                                 ? mBlockReflowState->frame
>                                 : psd->mFrame->mFrame));
>       printf(": trim of ");
>       nsFrame::ListTag(stdout, pfd->mFrame);
>       printf(" returned %d\n", trimOutput.mDeltaWidth);
> #endif
>-      if (trimOutput.mLastCharIsJustifiable && pfd->mJustificationNumSpaces > 0) {
>-        pfd->mJustificationNumSpaces--;
>-      }
>-      
>+
>       if (trimOutput.mChanged) {
>         pfd->SetFlag(PFD_RECOMPUTEOVERFLOW, true);
>       }
> 
>       if (trimOutput.mDeltaWidth) {

Could you add comment above this if statement for explaining this means that the frame has trimmed space?

>         pfd->mBounds.ISize(lineWM) -= trimOutput.mDeltaWidth;
> 
>+        if (!pfd->mNext) {
>+          // If any trailing space is trimmed in the last frame in a line,
>+          // the justification point generated by the space should be
>+          // removed as well.
>+          JustificationCount& justificationCount = pfd->mJustificationCount;
>+          if (justificationCount.mInnerOpportunities > 0) {
>+            justificationCount.mInnerOpportunities--;
>+          } else if (justificationCount.mIsStartJustifiable) {
>+            justificationCount.mIsStartJustifiable = false;
>+          }

So, could be JustificationCound::CancelOpportunityForTrimmedSpace() or something?

>+int32_t
>+nsLineLayout::CountInnerExpansionOpportunities(PerSpanData* aPSD)

Ideally, "Justification" should be included into the name, but I have no idea...

>+  for (PerFrameData* pfd = aPSD->mFirstFrame; pfd; pfd = pfd->mNext) {
>+    JustificationCount& count = pfd->mJustificationCount;
>+    if (pfd->GetFlag(PFD_ISTEXTFRAME)) {
>+      result += count.mInnerOpportunities;
>+    } else if (pfd->mSpan) {
>+      PerSpanData* span = pfd->mSpan;
>+      result += CountInnerExpansionOpportunities(span);
>+      count.mIsStartJustifiable =
>+        span->mFirstFrame && span->mFirstFrame->IsStartJustifiable();
>+      count.mIsEndJustifiable =
>+        span->mLastFrame && span->mLastFrame->IsEndJustifiable();

Thanks! It's very clear what this code does!!

>+    JustificationFlags& flags = pfd->mJustificationFlags;
>+    flags = JustificationFlags();
>+    if (count.mIsStartJustifiable && pfd->mPrev) {
>+      SetJustificationSpaceAtStart(
>+          flags, pfd->mPrev->mJustificationFlags, result);

"Adjust" might be better? (instead of "Set")

> nscoord 
> nsLineLayout::ApplyFrameJustification(PerSpanData* aPSD, FrameJustificationState* aState)

>+        if (aState->mSpaces.mCount > 0 &&
>+            aState->mWidth.mAvailable > 0) {

Could be FrameJustificationState::IsJustifiable() or something?

>+          aState->mSpaces.mHandled += CountExpansionSpaces(count, flags);
>+          nscoord newAllocated =
>+            (aState->mWidth.mAvailable * aState->mSpaces.mHandled) /
>+            aState->mSpaces.mCount;
>+          dw = newAllocated - aState->mWidth.mConsumed;
>+          aState->mWidth.mConsumed = newAllocated;

I guess that this can be FrameJustificationState::Consume(const JustificationCount& aCount, const JustificationFlags& aFlags). And return the dw value as its result.

>+      } else if (pfd->mSpan && pfd->mSpan->mFirstFrame) {
>+        PerSpanData* span = pfd->mSpan;
>+        span->mFirstFrame->mJustificationFlags.CopySpacesAtStart(flags);
>+        span->mLastFrame->mJustificationFlags.CopySpacesAtEnd(flags);
>+        dw = ApplyFrameJustification(span, aState);
>       }

Could you explain why you copy the flags with comment?

And do you need to check if mFirstFrame is not nullptr? It was not checked. Does this work fine with empty <span></span>?

>diff --git a/layout/generic/nsLineLayout.h b/layout/generic/nsLineLayout.h
>@@ -567,28 +578,28 @@ protected:
>   void PlaceTopBottomFrames(PerSpanData* psd,
>                             nscoord aDistanceFromStart,
>                             nscoord aLineBSize);
> 
>   void RelativePositionFrames(PerSpanData* psd, nsOverflowAreas& aOverflowAreas);
> 
>   bool TrimTrailingWhiteSpaceIn(PerSpanData* psd, nscoord* aDeltaISize);
> 
>-  void ComputeJustificationWeights(PerSpanData* psd, int32_t* numSpaces, int32_t* numLetters);
>-
>-  struct FrameJustificationState {
>-    int32_t mTotalNumSpaces;
>-    int32_t mTotalNumLetters;
>-    nscoord mTotalWidthForSpaces;
>-    nscoord mTotalWidthForLetters;
>-    int32_t mNumSpacesProcessed;
>-    int32_t mNumLettersProcessed;
>-    nscoord mWidthForSpacesProcessed;
>-    nscoord mWidthForLettersProcessed;
>+  struct FrameJustificationState
>+  {
>+    struct {

nit: put the |{| to the next line below the "s".

>+      int32_t mCount;
>+      int32_t mHandled;
>+    } mSpaces;
>+    struct {

ditto.

>+      nscoord mAvailable;
>+      nscoord mConsumed;
>+    } mWidth;
>   };

>diff --git a/layout/generic/nsTextFrame.cpp b/layout/generic/nsTextFrame.cpp
>+  if (!mJustificationFlagsArray.IsEmpty() &&
>+      mJustificationFlagsArray.LastElement().mSpaceAtEnd) {
>+    // We counted the expansion opportunity after the last character,
>+    // but it is not an inner opportunity.
>+    mJustificationCount.mInnerOpportunities--;

Could you add MOZ_ASSERT(mJustificationCount.mInnerOpportunities > 0) before this line?

>+    mJustificationCount.mIsEndJustifiable = true;
>+    mJustificationFlagsArray.LastElement().mSpaceAtEnd = false;
>+    mJustificationFlagsArray.LastElement().mExtraSpaceAtEnd = false;

Setting mIsEndJustifiable true, but setting mSpaceAtEnd and mExtraSpaceAtEnd false may make developers confused. Please add a comment here.

>@@ -3071,44 +3120,30 @@ PropertyProvider::GetSpacingInternal(uin
>   // Now add in justification spacing
>   if (mJustificationSpacing) {
>+    // If there is any spaces trimmed at the end, aStart + aLength may
>+    // be larger than the flags array. When that happens, we can simply
>+    // ignore those spaces.
>+    auto end = std::min(aStart + aLength,
>+                        mJustificationArrayStart +
>+                        uint32_t(mJustificationFlagsArray.Length()));

nit: add more 2 spaces into the indent of the last line because it's a part of previous line.


Looks much simpler than previos patches to me! Thank you. Let me check the new patch, then, perhaps, I'll give r+ and request additional review to roc. And when you request next review, could you post tryserver to create a Windows build? I'd like to check the actual behavior before giving r+.

# Sorry for the delay to review due to national holiday of Japan.

        Attachment #8502299 -
        Flags: review?(masayuki)

    
    

    
  
Comment on attachment 8492739 [details] [diff] [review]
patch for test

And could you test with empty spans?

E.g.,
おは<span></span>よう<span><span></span>ござ<span></span></span>います

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #54)
> 
> >+      } else if (pfd->mSpan && pfd->mSpan->mFirstFrame) {
> >+        PerSpanData* span = pfd->mSpan;
> >+        span->mFirstFrame->mJustificationFlags.CopySpacesAtStart(flags);
> >+        span->mLastFrame->mJustificationFlags.CopySpacesAtEnd(flags);
> >+        dw = ApplyFrameJustification(span, aState);
> >       }
> 
> Could you explain why you copy the flags with comment?
> 
> And do you need to check if mFirstFrame is not nullptr? It was not checked.
> Does this work fine with empty <span></span>?

In the previous code, if it is an empty span, ApplyFrameJustifcation just skips the whole loop and returns 0, hence for this case, the two versions are effectively equal. It processes empty spans just like what is done before.

> >+    mJustificationCount.mIsEndJustifiable = true;
> >+    mJustificationFlagsArray.LastElement().mSpaceAtEnd = false;
> >+    mJustificationFlagsArray.LastElement().mExtraSpaceAtEnd = false;
> 
> Setting mIsEndJustifiable true, but setting mSpaceAtEnd and mExtraSpaceAtEnd
> false may make developers confused. Please add a comment here.

I'd like to get rid of the confusing code directly, since the flags of last element would be set again in SetupJustificationSpacing according to flags of the text frame.

> Looks much simpler than previos patches to me! Thank you. Let me check the
> new patch, then, perhaps, I'll give r+ and request additional review to roc.
> And when you request next review, could you post tryserver to create a
> Windows build? I'd like to check the actual behavior before giving r+.

Thanks for your reviewing.

> # Sorry for the delay to review due to national holiday of Japan.

No worries.
Flags: needinfo?(roc)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Fix the uninitialised value revealed by valgrind test.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b72df8522720

        Attachment #8505867 -
        Attachment is obsolete: true

        Attachment #8505867 -
        Flags: review?(masayuki)

        Attachment #8506545 -
        Flags: review?(masayuki)

    
    

    
  
Comment on attachment 8506545 [details] [diff] [review]
patch

I've not checked the patch yet, but I tested your tryserver build. However, it has some bugs:
http://jsfiddle.net/jfzvyywv/5/

1. With <li> elements? Or with the indent of <li>? Each last character isn't aligned to the end of its line.
2. Empty <span> element causes extra space. This shouldn't be. (#2, #10, #15)

        Attachment #8506545 -
        Flags: review?(masayuki) → review-

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #60)
> Comment on attachment 8506545 [details] [diff] [review]
> patch
> 
> I've not checked the patch yet, but I tested your tryserver build. However,
> it has some bugs:
> http://jsfiddle.net/jfzvyywv/5/
> 
> 1. With <li> elements? Or with the indent of <li>? Each last character isn't
> aligned to the end of its line.

I'll check this.

> 2. Empty <span> element causes extra space. This shouldn't be. (#2, #10, #15)

This is a known issue, and I don't think we should solve it for now. On the one hand, it is a rare case in normal use, but for dealing with this, the algorithm would be much more complex. On the other hand, WebKit and Blink have the same effect, and the spec doesn't have specific requirements on this.

If we really want to solve it, I think it would be better to open a follow-up bug after this bug being solved.

    
    

    
  
BTW, do you think we should apply justification between the bullet and the item text? Chrome does so but we don't.
Flags: needinfo?(masayuki)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
OK, I solve the both problems together.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=46e4d4eb0142

        Attachment #8506545 -
        Attachment is obsolete: true

        Attachment #8507286 -
        Flags: review?(masayuki)

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #62)
> BTW, do you think we should apply justification between the bullet and the
> item text? Chrome does so but we don't.

I don't think so. Bullets are not a part of each line. I believe that other browsers should fix it. fantasai, how about you?
Flags: needinfo?(masayuki)

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #63)
> Created attachment 8507286 [details] [diff] [review]
> patch
> 
> OK, I solve the both problems together.
> 
> try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=46e4d4eb0142

Looks fine! Great. (FYI: The empty span issue was a regression of your patch. It's not reproduced with current Nightly)

    
    

    
  
No, we should not adjust spacing between the bullet and the main text. Please make sure that test ends up in the w3c export directory. :)
Flags: needinfo?(fantasai.bugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8507286 -
        Attachment is obsolete: true

        Attachment #8507286 -
        Flags: review?(masayuki)

        Attachment #8507580 -
        Flags: review?(masayuki)

    
  
Blocks: ruby-align

    
    

    
  
(In reply to fantasai from comment #66)
> No, we should not adjust spacing between the bullet and the main text.

Thanks.

> Please make sure that test ends up in the w3c export directory. :)

Hmm, about the text-align-last: justify; isn't rendered as the test described both on patched build and Nightly :-( It must be another bug of us or the test...
http://test.csswg.org/suites/css3-text/nightly-unstable/html/text-align-last-006.htm

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #68)
> (In reply to fantasai from comment #66)
> > No, we should not adjust spacing between the bullet and the main text.
> 
> Thanks.
> 
> > Please make sure that test ends up in the w3c export directory. :)
> 
> Hmm, about the text-align-last: justify; isn't rendered as the test
> described both on patched build and Nightly :-( It must be another bug of us
> or the test...
> http://test.csswg.org/suites/css3-text/nightly-unstable/html/text-align-last-
> 006.htm

It is only because that the test doesn't use -moz- prefix. We haven't yet unprefixed it.

    
    

    
  
Comment on attachment 8507580 [details] [diff] [review]
patch

> nscoord 
> nsLineLayout::ApplyFrameJustification(PerSpanData* aPSD, FrameJustificationState* aState)
> {
>   NS_ASSERTION(aPSD, "null arg");
>   NS_ASSERTION(aState, "null arg");
> 
>   nscoord deltaICoord = 0;
>   for (PerFrameData* pfd = aPSD->mFirstFrame; pfd != nullptr; pfd = pfd->mNext) {
>     // Don't reposition bullets (and other frames that occur out of X-order?)
>     if (!pfd->GetFlag(PFD_ISBULLET)) {
>       nscoord dw = 0;
>       WritingMode lineWM = mRootSpan->mWritingMode;
>+      const JustificationFlags& flags = pfd->mJustificationFlags;
>+      const JustificationCount& count = pfd->mJustificationCount;
> 
>       pfd->mBounds.IStart(lineWM) += deltaICoord;
> 
>       if (true == pfd->GetFlag(PFD_ISTEXTFRAME)) {
>+        if (aState->mSpaces.mCount > 0 &&
>+            aState->mWidth.mAvailable > 0) {

IsJustifiable()?

>+          // Set corresponding state bits here, so that the text frame
>+          // knows how it should add spaces at its sides.
>+          nsIFrame* frame = pfd->mFrame;
>+          frame->RemoveStateBits(TEXT_JUSTIFICATION_SPACE_AT_START |
>+                                 TEXT_JUSTIFICATION_EXTRA_SPACE_AT_START |
>+                                 TEXT_JUSTIFICATION_SPACE_AT_END |
>+                                 TEXT_JUSTIFICATION_EXTRA_SPACE_AT_END);
>+          if (flags.mSpaceAtStart) {
>+            frame->AddStateBits(TEXT_JUSTIFICATION_SPACE_AT_START);
>+            if (flags.mExtraSpaceAtStart) {
>+              frame->AddStateBits(TEXT_JUSTIFICATION_EXTRA_SPACE_AT_START);
>+            }
>+          }
>+          if (flags.mSpaceAtEnd) {
>+            frame->AddStateBits(TEXT_JUSTIFICATION_SPACE_AT_END);
>+            if (flags.mExtraSpaceAtEnd) {
>+              frame->AddStateBits(TEXT_JUSTIFICATION_EXTRA_SPACE_AT_END);
>+            }
>+          }

Could be a method of nsTextFrame which takes JustificationFlags as an argument? E.g.,

static_cast<nsTextFrame*>(pfd->mFlame)->SetJustificationStateBits(flags);

>@@ -389,39 +391,41 @@ protected:
>     nsOverflowAreas mOverflowAreas;
> 
>     // From reflow-state
>     mozilla::LogicalMargin mMargin;
>     mozilla::LogicalMargin mBorderPadding;
>     mozilla::LogicalMargin mOffsets;
> 
>     // state for text justification
>+    mozilla::JustificationCount mJustificationCount;
>+    mozilla::JustificationFlags mJustificationFlags;
>     
> // PerFrameData flags
> #define PFD_RELATIVEPOS                 0x00000001
> #define PFD_ISTEXTFRAME                 0x00000002
> #define PFD_ISNONEMPTYTEXTFRAME         0x00000004
> #define PFD_ISNONWHITESPACETEXTFRAME    0x00000008
> #define PFD_ISLETTERFRAME               0x00000010
> #define PFD_RECOMPUTEOVERFLOW           0x00000020
> #define PFD_ISBULLET                    0x00000040
> #define PFD_SKIPWHENTRIMMINGWHITESPACE  0x00000080
>+#define PFD_ISEMPTY                     0x00000100
>+#define PFD_LASTFLAG                    PFD_ISEMPTY
> 
>+    // Other state we use
>+    uint16_t mFlags;

Although, this isn't a scope of this bug, perhaps, this should be bool flags instead of hacky bit fields...

>+void
>+PropertyProvider::ComputeJustification(int32_t aOffset, int32_t aLength)
>+{
>+  bool isCJ = IsChineseOrJapanese(mFrame);

According to the document of text-justiy: auto;, we should include some South Asian Scripts.
http://dev.w3.org/csswg/css-text-3/#valdef-text-justify-auto

Of course, it's not a scope of this bug.

>+  nsSkipCharsRunIterator
>+    run(mStart, nsSkipCharsRunIterator::LENGTH_INCLUDES_SKIPPED, aLength);
>+  run.SetOriginalOffset(aOffset);
>+  mJustificationArrayStart = run.GetSkippedOffset();
>+
>+  MOZ_ASSERT(mJustificationFlagsArray.IsEmpty());
>+  mJustificationFlagsArray.SetCapacity(aLength);
>+  while (run.NextRun()) {
>+    uint32_t originalOffset = run.GetOriginalOffset();
>+    uint32_t skippedOffset = run.GetSkippedOffset();
>+    uint32_t length = run.GetRunLength();
>+    mJustificationFlagsArray.SetLength(
>+        skippedOffset + length - mJustificationArrayStart);

nit: indent should be 2 spaces.

>+
>+    gfxSkipCharsIterator iter = run.GetPos();
>+    for (uint32_t i = 0; i < length; ++i) {
>+      uint32_t offset = originalOffset + i;
>+      if (!IsJustifiableCharacter(mFrag, offset, isCJ)) {
>+        continue;
>+      }
>+
>+      iter.SetOriginalOffset(offset);
>+
>+      FindClusterStart(mTextRun, originalOffset, &iter);
>+      uint32_t firstCharOffset = iter.GetSkippedOffset();
>+      uint32_t firstChar = firstCharOffset > mJustificationArrayStart ?
>+        firstCharOffset - mJustificationArrayStart : 0;
>+      if (firstChar == 0) {

nit: !firstChar

>+        mJustificationCount.mIsStartJustifiable = true;
>+      } else {
>+        JustificationUtils::AdjustSpaceAtStart(
>+            mJustificationFlagsArray[firstChar],
>+            mJustificationFlagsArray[firstChar - 1],
>+            mJustificationCount.mInnerOpportunities);

nit: indent should be 2 spaces.

>+      }
>+
>+      FindClusterEnd(mTextRun, originalOffset + length, &iter);
>+      uint32_t lastChar = iter.GetSkippedOffset() - mJustificationArrayStart;
>+      JustificationUtils::SetSpaceAtEnd(
>+          mJustificationFlagsArray[lastChar],
>+          mJustificationCount.mInnerOpportunities);

ditto

>@@ -3072,44 +3122,30 @@ PropertyProvider::GetSpacingInternal(uin
>     if (mTabWidths) {
>       mTabWidths->ApplySpacing(aSpacing,
>                                aStart - mStart.GetSkippedOffset(), aLength);
>     }
>   }
> 
>   // Now add in justification spacing
>   if (mJustificationSpacing) {
>+    // If there is any spaces trimmed at the end, aStart + aLength may
>+    // be larger than the flags array. When that happens, we can simply
>+    // ignore those spaces.
>+    auto end = std::min(aStart + aLength,
>+                        mJustificationArrayStart +
>+                          uint32_t(mJustificationFlagsArray.Length()));

nit: use static_cast.

> PropertyProvider::SetupJustificationSpacing(bool aPostReflow)
> {
>   NS_PRECONDITION(mLength != INT32_MAX, "Can't call this with undefined length");
> 
>   if (!(mFrame->GetStateBits() & TEXT_JUSTIFICATION_ENABLED))
>     return;
> 
>   gfxSkipCharsIterator start(mStart), end(mStart);
>   // We can't just use our mLength here; when InitializeForDisplay is
>   // called with false for aTrimAfter, we still shouldn't be assigning
>   // justification space to any trailing whitespace.
>   nsTextFrame::TrimmedOffsets trimmed =
>     mFrame->GetTrimmedOffsets(mFrag, true, aPostReflow);
>   end.AdvanceOriginal(trimmed.mLength);
>   gfxSkipCharsIterator realEnd(end);
>+  ComputeJustification(start.GetOriginalOffset(),
>+                       end.GetOriginalOffset() - start.GetOriginalOffset());
>+
>+  JustificationFlags flags;
>+  nsFrameState stateBits = mFrame->GetStateBits();
>+  if (stateBits & TEXT_JUSTIFICATION_SPACE_AT_START) {
>+    flags.mSpaceAtStart = true;
>+    if (stateBits & TEXT_JUSTIFICATION_EXTRA_SPACE_AT_START) {
>+      flags.mExtraSpaceAtStart = true;
>+    }
>+  }
>+  if (stateBits & TEXT_JUSTIFICATION_SPACE_AT_END) {
>+    flags.mSpaceAtEnd = true;
>+    if (stateBits & TEXT_JUSTIFICATION_EXTRA_SPACE_AT_END) {
>+      flags.mExtraSpaceAtEnd = true;
>+    }
>+  }
>+
>+  int32_t spaces =
>+    JustificationUtils::CountExpansionSpaces(mJustificationCount, flags);
>+  if (spaces == 0 || mJustificationFlagsArray.IsEmpty()) {

nit: !spaces

>@@ -544,16 +541,18 @@ protected:
>   // This does *not* indicate the length of text currently mapped by the frame;
>   // instead it's a hint saying that this frame *wants* to map this much text
>   // so if we create a new continuation, this is where that continuation should
>   // start.
>   int32_t     mContentLengthHint;
>   nscoord     mAscent;
>   gfxTextRun* mTextRun;
> 
>+  mozilla::JustificationFlags mJustificationFlags;
>+

Don't you forget to remove this?

Otherwise, looks okay to me. Please ask additional review to roc with new patch. Thank you very much for your great work!!

        Attachment #8507580 -
        Flags: review?(masayuki) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch, r=masayuki (obsolete)
        — 
          — Splinter Review
      

    


  
I think the most critical part in the patch need to have additional review is the added state bits for text frames.

        Attachment #8507580 -
        Attachment is obsolete: true

        Attachment #8507676 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8507676 [details] [diff] [review]
patch, r=masayuki

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

::: layout/generic/JustificationUtils.h
@@ +27,5 @@
> +struct JustificationCount
> +{
> +  // Number of expansion opportunities inside a given range of clusters.
> +  // It does not include any points before or after the range of clusters.
> +  int32_t mInnerOpportunities;

Needs a bit more explanation, maybe with some examples. A JustificationCount will be associated with a range of clusters, right? If that range of clusters includes a trailing space character, does that space count as an inner opportunity?

@@ +31,5 @@
> +  int32_t mInnerOpportunities;
> +  // Whether it is possible that there are insert points before or
> +  // after the range of clusters.
> +  bool mIsStartJustifiable;
> +  bool mIsEndJustifiable;

What do these mean, more precisely? Can you give some examples?

@@ +41,5 @@
> +  {
> +  }
> +
> +  // Claim that the last opportunity should be cancelled
> +  // because the tailing space just gets trimmed.

trailing

::: layout/generic/nsFrameStateBits.h
@@ +416,5 @@
> +// See nsLineLayout::ApplyFrameJustification and JustificationUtils.h.
> +FRAME_STATE_BIT(Text, 33, TEXT_JUSTIFICATION_SPACE_AT_START)
> +FRAME_STATE_BIT(Text, 34, TEXT_JUSTIFICATION_EXTRA_SPACE_AT_START)
> +FRAME_STATE_BIT(Text, 35, TEXT_JUSTIFICATION_SPACE_AT_END)
> +FRAME_STATE_BIT(Text, 36, TEXT_JUSTIFICATION_EXTRA_SPACE_AT_END)

These bits conflict with Generic bits NS_FRAME_DRAWING_AS_PAINTSERVER and NS_FRAME_UPDATE_LAYER_TREE :-(

I don't think we have any implementation-reserved bits available for textframes. Since justification doesn't need to be super-well optimized maybe we can use a FrameProperty to hold the bits for justification.

::: layout/generic/nsLineLayout.h
@@ +447,5 @@
> +    {
> +      return mJustificationCount.mIsEndJustifiable;
> +    }
> +
> +    bool ParticipatesJustification() const

ParticipatesInJustification

        Attachment #8507676 -
        Flags: review?(roc) → review-

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #72)
> Comment on attachment 8507676 [details] [diff] [review]
> patch, r=masayuki
> 
> ::: layout/generic/nsFrameStateBits.h
> @@ +416,5 @@
> > +// See nsLineLayout::ApplyFrameJustification and JustificationUtils.h.
> > +FRAME_STATE_BIT(Text, 33, TEXT_JUSTIFICATION_SPACE_AT_START)
> > +FRAME_STATE_BIT(Text, 34, TEXT_JUSTIFICATION_EXTRA_SPACE_AT_START)
> > +FRAME_STATE_BIT(Text, 35, TEXT_JUSTIFICATION_SPACE_AT_END)
> > +FRAME_STATE_BIT(Text, 36, TEXT_JUSTIFICATION_EXTRA_SPACE_AT_END)
> 
> These bits conflict with Generic bits NS_FRAME_DRAWING_AS_PAINTSERVER and
> NS_FRAME_UPDATE_LAYER_TREE :-(
> 
> I don't think we have any implementation-reserved bits available for
> textframes. Since justification doesn't need to be super-well optimized
> maybe we can use a FrameProperty to hold the bits for justification.

Yes, that's great! I was looking for something like this to hold the data instead of the state bits, since I might soon need more data to be stored with text frames.

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #70)
> Comment on attachment 8507580 [details] [diff] [review]
> patch
> 
> >@@ -389,39 +391,41 @@ protected:
> >     nsOverflowAreas mOverflowAreas;
> > 
> >     // From reflow-state
> >     mozilla::LogicalMargin mMargin;
> >     mozilla::LogicalMargin mBorderPadding;
> >     mozilla::LogicalMargin mOffsets;
> > 
> >     // state for text justification
> >+    mozilla::JustificationCount mJustificationCount;
> >+    mozilla::JustificationFlags mJustificationFlags;
> >     
> > // PerFrameData flags
> > #define PFD_RELATIVEPOS                 0x00000001
> > #define PFD_ISTEXTFRAME                 0x00000002
> > #define PFD_ISNONEMPTYTEXTFRAME         0x00000004
> > #define PFD_ISNONWHITESPACETEXTFRAME    0x00000008
> > #define PFD_ISLETTERFRAME               0x00000010
> > #define PFD_RECOMPUTEOVERFLOW           0x00000020
> > #define PFD_ISBULLET                    0x00000040
> > #define PFD_SKIPWHENTRIMMINGWHITESPACE  0x00000080
> >+#define PFD_ISEMPTY                     0x00000100
> >+#define PFD_LASTFLAG                    PFD_ISEMPTY
> > 
> >+    // Other state we use
> >+    uint16_t mFlags;
> 
> Although, this isn't a scope of this bug, perhaps, this should be bool flags
> instead of hacky bit fields...

Should we have a new bug for this? I guess it would be painful to make them bool flags and init them manually.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch, r=masayuki (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8507676 -
        Attachment is obsolete: true

        Attachment #8508438 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8508438 [details] [diff] [review]
patch, r=masayuki

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

::: layout/generic/JustificationUtils.h
@@ +41,5 @@
> +  // Number of expansion opportunities inside a given range of
> +  // clusters. It does not include any opportunities before or after
> +  // the range of clusters.
> +  int32_t mInnerOpportunities;
> +  // Whether it is possible that there are expansion opportunities

I think you should remove "it is possible that"

@@ +70,5 @@
> +    }
> +  }
> +};
> +
> +struct JustificationFlags

Need comments explaining what this is for and what the flags mean.

@@ +122,5 @@
> +  // justification flags of two adjacent parts.
> +
> +  static void AdjustSpaceAtStart(JustificationFlags& aFlags,
> +                                 JustificationFlags& aPrevFlags,
> +                                 int32_t& aCount)

Please add comments explaining what these functions mean.

Would it make sense to make these member functions of JustificationFlags?

        Attachment #8508438 -
        Flags: review?(roc) → review-

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #76)
> Comment on attachment 8508438 [details] [diff] [review]
> patch, r=masayuki
> 
> Review of attachment 8508438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/JustificationUtils.h
> @@ +41,5 @@
> > +  // Number of expansion opportunities inside a given range of
> > +  // clusters. It does not include any opportunities before or after
> > +  // the range of clusters.
> > +  int32_t mInnerOpportunities;
> > +  // Whether it is possible that there are expansion opportunities
> 
> I think you should remove "it is possible that"

I don't think so. There is not always an expansion opportunity when the given side is justifiable. The most obvious case is the line start and line end.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #76)
> Comment on attachment 8508438 [details] [diff] [review]
> patch, r=masayuki
> 
> Review of attachment 8508438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +70,5 @@
> > +    }
> > +  }
> > +};
> > +
> > +struct JustificationFlags
> 
> Need comments explaining what this is for and what the flags mean.

Isn't it obvious enough with the names and the explaination of the algorithm above? I feel additional comments here are redundant.

> 
> @@ +122,5 @@
> > +  // justification flags of two adjacent parts.
> > +
> > +  static void AdjustSpaceAtStart(JustificationFlags& aFlags,
> > +                                 JustificationFlags& aPrevFlags,
> > +                                 int32_t& aCount)
> 
> Please add comments explaining what these functions mean.

I feel it's hard to explain their function individually. But they are short enough and properly commented in the code. I don't think we need more comments on them.

> Would it make sense to make these member functions of JustificationFlags?

It might make sense, but I prefer keeping them where they are now, because they not only change the flags, but also participate in the algorithm and count the expansion opportunities for their caller. They were inlined in the callers directly in former patches, and be separated out as functions because they are common parts in two places.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch, r=masayuki (obsolete)
        — 
          — Splinter Review
      

    


  
Adjust some comments.

        Attachment #8508438 -
        Attachment is obsolete: true

        Attachment #8511741 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8511741 [details] [diff] [review]
patch, r=masayuki

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

::: layout/generic/JustificationUtils.h
@@ +26,5 @@
> + * In the following examples, we use "A" to represent justifiable
> + * clusters, and "a" for non-justifiable ones.
> + *
> + * If we have a string "aaAA", the inner expansion opportunites (marked
> + * as stars) would be "a a*A*A".  In addition to the inner expansion

Should this be aa*A*A ?

@@ +35,5 @@
> + * If a line is "AaaAA", the final result would be "A>>aa<<A><A", where
> + * ">" means a space expanded from the cluster before, and "<" means one
> + * from the cluster after. Note that, although the string of this line
> + * is justifiable in both sides according to the rule above, there are
> + * no expansion opportunities, because they are line ends.

You use * for expansion opportunities in the previous paragraph and >> and < in this paragraph. Can you be consistent and use >> and < in the previous paragraph instead of *?

@@ +74,5 @@
> +  }
> +};
> +
> +struct JustificationFlags
> +{

This still needs documentation. I had to read far ahead in the patch to realize that these flags are actually per-(non-collapsed-)character! That, at least, should be documented here.

@@ +76,5 @@
> +
> +struct JustificationFlags
> +{
> +  bool mSpaceAtStart      : 1;
> +  bool mExtraSpaceAtStart : 1;

It's not clear what mExtraSpaceAtStart means, even after reading the explanation at the start of the file.

Also, these record expansion opportunities, not actual 0x20 space characters, right? If so we should change the names.

@@ +130,5 @@
> +  // flags according to the flags of the previous adjacent part, and
> +  // increase the count if it is a new opportunity.
> +  static void AdjustSpaceAtStart(JustificationFlags& aFlags,
> +                                 JustificationFlags& aPrevFlags,
> +                                 int32_t& aCount)

It's still not very clear what this function does. It seems like we're combining justification data from a previous text run with justification data for the next text run?

I'm sorry to be picky about the comments here but making this code easy to understand now will save us a lot of time and effort in the future.

It would probably help if the explanation of the algorithm above referenced the actual types and fields that store data used by the algorithm.

::: layout/generic/nsLineLayout.cpp
@@ +2484,5 @@
> +  struct
> +  {
> +    nscoord mAvailable;
> +    nscoord mConsumed;
> +  } mWidth;

These need documentation too.

        Attachment #8511741 -
        Flags: review?(roc) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch, r=masayuki (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8511741 -
        Attachment is obsolete: true

        Attachment #8513122 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8513122 [details] [diff] [review]
patch, r=masayuki

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

::: layout/generic/JustificationUtils.h
@@ +50,5 @@
> + * case is similar for its end side.  The first and the last justifi-
> + * cation units of a span inherit the justification spaces at start and
> + * end respectively from the span. All justification spaces are finally
> + * applied to clusters, where they take their effect. The struct
> + * JustificationFlags is used to track the information in this phase.

Are we doing this because, given the text "a b c", we want selection of "b" to include half the space between "a" and "b" and on the other side half the space between "b" and "c"? If so, please say so.

I still find this whole patch very hard to understand. The first phase makes sense. The second phase is very confusing. Here are some suggestions:

-- I think we should rename JustificationCount to JustificationOpportunities, since it's not just a count.

-- I think we should have a method that just does "phase one", i.e. it gets called for each span and does nothing but compute the JustificationDataForSpan for that span, calling itself recursively to walk over the children.

-- I think we should rename JustificationFlags to JustificationAssignment. I think it should include the number of inner opportunities actually assigned (an integer), and the number of half-opportunities assigned to each end; it would be clearest if the latter fields are floats.

-- I think we should have a specific method for phase two, that gets called for each span and computes for each span (and ultimately each frame) its JustificationAssignment.

-- I think we should attach the JustificationAssignment to the frame as a FrameProperty, just dynamically allocated as a struct.

How does that sound?

@@ +60,5 @@
> +  // any opportunities between this span and the one before or after.
> +  int32_t mInnerOpportunities;
> +  // The justifiability of the start and end sides of the span.
> +  bool mIsStartJustifiable;
> +  bool mIsEndJustifiable;

Somewhere I would expect to see a method that takes two JustificationCounts, one for each part of a consecutive run of text, and merges them, but I don't see it anywhere :-(.

@@ +150,5 @@
> +  // it forms a new opportunity.
> +  static void AdjustSpaceAtStart(JustificationFlags& aFlags,
> +                                 JustificationFlags& aPrevFlags,
> +                                 int32_t& aCount)
> +  {

I still find these methods hard to understand :-(.

        Attachment #8513122 -
        Flags: review?(roc) → review-

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #82)
> Comment on attachment 8513122 [details] [diff] [review]
> patch, r=masayuki
> 
> Review of attachment 8513122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still find this whole patch very hard to understand. The first phase makes
> sense. The second phase is very confusing. Here are some suggestions:
> 
> -- I think we should rename JustificationCount to
> JustificationOpportunities, since it's not just a count.
> 
> -- I think we should have a method that just does "phase one", i.e. it gets
> called for each span and does nothing but compute the
> JustificationDataForSpan for that span, calling itself recursively to walk
> over the children.

I think completely separate the two phases may complexify the code.

> -- I think we should rename JustificationFlags to JustificationAssignment. I
> think it should include the number of inner opportunities actually assigned
> (an integer), 

I don't think including number of inner opportunities is useful, since we always have to recaculate it inside text frames when computing the assignment for clusters. 

> and the number of half-opportunities assigned to each end; it
> would be clearest if the latter fields are floats.

We can store sufficient information in 4 bits, why should we store it in 2 floats? I cannot agree with this suggestion.

> -- I think we should attach the JustificationAssignment to the frame as a
> FrameProperty, just dynamically allocated as a struct.

I don't think so. 4 bits are enough for that information. Dynamically allocating has no sense other than adding the code complexity and reducing the performance.

    
    

    
  
I'd prefer make the comment more clear before changing the code.

Would the folloing comment make more sense to you?

/**
 * Jutification Algorithm
 *
 * The justification algorithm is based on expansion opportunities
 * between justifiable clusters.  By this algorithm, there is one
 * expansion opportunity at each side of a justifiable cluster, and
 * at most one opportunity between two clusters. For example, if there
 * is a line in a Chinese document is: "你好世界hello world", then
 * the expansion opportunities would be:
 *
 *                    你 好 世 界 hello ' ' world
 *                      ^  ^  ^  ^     ^   ^
 * The spacing left in a line will then be distributed equally to each
 * opportunities. Because we want that, only justifiable clusters get
 * expanded, and the split point between two justifiable clusters would
 * be at the center of them, each expansion opportunities will be filled
 * by two justification space. The example above would be:
 *
 *              你 | 好 | 世 | 界  |hello|  ' '  |world
 *
 * For a flatten sequence of clusters, this algorithm could be completed
 * in one pass. It checks whether a cluster is justifiable, and assigns
 * justification spaces to them. Since it is unknown whether the cluster
 * after is justifiable or not, every time when a justifiable is found,
 * the two justification spaces of the expansion opportunity after it
 * are both assigned to it. When a new justifiable cluster is found, it
 * will be checked whether the cluster before is also justifiable, and
 * then the assigned space would be adjusted. This procedure is done by
 * JustificationUtils::SetSpaceAtEnd and AdjustSpaceAtStart.
 *
 * It becomes more complex when lines consist of nested spans instead of
 * a flatten sequence. To process spans, the algorithm is divided into
 * two phases. Since spans and clusters play similar roles in many
 * places in this algorithm, we call them justification units below.
 *
 * In the first phase, the algorithm for flatten sequences described
 * above is applied to each span. After that, justification spaces of
 * opportunities between units inside a same span get assigned. Then
 * in the second phase, justification spaces assigned to a span are
 * recursively assigned to the first and last units inside the span.
 * The space assignment is recoreded in structure JustificationFlags.
 *
 * In the practical code of the algorithm, there are some extra works
 * in the two phases. In the first phase, expansion opportunities are
 * counted for calculating the size of each justification space. And in
 * the second phase, spans are expanded accordingly.
 */
Flags: needinfo?(roc)

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #84)
>  *                    你 好 世 界 hello ' ' world
>  *                      ^  ^  ^  ^     ^   ^

Unfortunately this is difficult to read because in various renderings (Firefox, eclipse, gedit) the ^ characters end up misaligned because the Chinese characters are not truly monospace :-(. Either use inline marks like you had before, or use Latin characters in place of the Chinese characters.
Flags: needinfo?(roc)

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #85)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #84)
> >  *                    你 好 世 界 hello ' ' world
> >  *                      ^  ^  ^  ^     ^   ^
> 
> Unfortunately this is difficult to read because in various renderings
> (Firefox, eclipse, gedit) the ^ characters end up misaligned because the
> Chinese characters are not truly monospace :-(. Either use inline marks like
> you had before, or use Latin characters in place of the Chinese characters.

OK, so I use inline marks now:

/**
 * Jutification Algorithm
 *
 * The justification algorithm is based on expansion opportunities
 * between justifiable clusters.  By this algorithm, there is one
 * expansion opportunity at each side of a justifiable cluster, and
 * at most one opportunity between two clusters. For example, if there
 * is a line in a Chinese document is: "你好世界hello world", then
 * the expansion opportunities (marked as '*') would be:
 *
 *                    你*好*世*界*hello*' '*world
 *                                          
 * The spacing left in a line will then be distributed equally to each
 * opportunities. Because we want that, only justifiable clusters get
 * expanded, and the split point between two justifiable clusters would
 * be at the center of them, each expansion opportunities will be filled
 * by two justification space. The example above would be:
 *
 *              你 | 好 | 世 | 界  |hello|  ' '  |world
 *
 * For a flatten sequence of clusters, this algorithm could be completed
 * in one pass. It checks whether a cluster is justifiable, and assigns
 * justification spaces to them. Since it is unknown whether the cluster
 * after is justifiable or not, every time when a justifiable is found,
 * the two justification spaces of the expansion opportunity after it
 * are both assigned to it. When a new justifiable cluster is found, it
 * will be checked whether the cluster before is also justifiable, and
 * then the assigned space would be adjusted. This procedure is done by
 * JustificationUtils::SetSpaceAtEnd and AdjustSpaceAtStart.
 *
 * It becomes more complex when lines consist of nested spans instead of
 * a flatten sequence. To process spans, the algorithm is divided into
 * two phases. Since spans and clusters play similar roles in many
 * places in this algorithm, we call them justification units below.
 *
 * In the first phase, the algorithm for flatten sequences described
 * above is applied to each span. After that, justification spaces of
 * opportunities between units inside a same span get assigned. Then
 * in the second phase, justification spaces assigned to a span are
 * recursively assigned to the first and last units inside the span.
 * The space assignment is recoreded in structure JustificationFlags.
 *
 * In the practical code of the algorithm, there are some extra works
 * in the two phases. In the first phase, expansion opportunities are
 * counted for calculating the size of each justification space. And in
 * the second phase, spans are expanded accordingly.
 */
Flags: needinfo?(roc)

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #84)
>  * Then
>  * in the second phase, justification spaces assigned to a span are
>  * recursively assigned to the first and last units inside the span.
>  * The space assignment is recoreded in structure JustificationFlags.

This isn't as clear as the rest of the comment. When you say "justification spaces assigned to a span", you don't mean all the spaces assigned to the span, you just mean the spaces assigned to its ends, right?

The way you've described the algorithm here suggests an alternative implementation that might be easier to understand: walk the spans recursively and keep track of the previous text frame's PFD. When you see the next PFD, you can find out what the next cluster after the previous text frame is (if there isn't one), and then you can assign justification space to the previous text frame. That way you should be able to make the code look very much like the one-pass algorithm for the flattened sequence of clusters. Does that make sense?

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #87)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #84)
> >  * Then
> >  * in the second phase, justification spaces assigned to a span are
> >  * recursively assigned to the first and last units inside the span.
> >  * The space assignment is recoreded in structure JustificationFlags.
> 
> This isn't as clear as the rest of the comment. When you say "justification
> spaces assigned to a span", you don't mean all the spaces assigned to the
> span, you just mean the spaces assigned to its ends, right?

Right.

> The way you've described the algorithm here suggests an alternative
> implementation that might be easier to understand: walk the spans
> recursively and keep track of the previous text frame's PFD. When you see
> the next PFD, you can find out what the next cluster after the previous text
> frame is (if there isn't one), and then you can assign justification space
> to the previous text frame. That way you should be able to make the code
> look very much like the one-pass algorithm for the flattened sequence of
> clusters. Does that make sense?

Yes, that makes sense to me.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8513122 -
        Attachment is obsolete: true

        Attachment #8514023 -
        Flags: review?(roc)

    
  
Flags: needinfo?(roc)

    
    

    
  
Comment on attachment 8514023 [details] [diff] [review]
patch

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

::: layout/generic/JustificationUtils.h
@@ +90,5 @@
> +{
> +  bool mSpaceAtStart      : 1;
> +  bool mExtraSpaceAtStart : 1;
> +  bool mSpaceAtEnd        : 1;
> +  bool mExtraSpaceAtEnd   : 1;

I still think it's going to be clearer to call this JustificationAssignment and use two uint8_ts (mSpacesAtStart, mSpacesAtEnd) instead of 4 bools. See below.

Also I think using the word "space" here is confusing since we use the word "space" for other things in text, especially actual space characters. Maybe "slots"? So "mSlotsAtStart"?

::: layout/generic/nsLineLayout.cpp
@@ +2432,5 @@
> +};
> +
> +int32_t
> +nsLineLayout::ComputeFrameJustification(PerSpanData* aPSD,
> +                                        JustificationComputationState& aState)

Somewhere you need to document what the return value of this function means.

@@ +2455,5 @@
>  
> +        JustificationFlags& flags = pfd->mJustificationFlags;
> +        if (count.mIsStartJustifiable && aState.mLastParticipant) {
> +          JustificationUtils::AdjustSpaceAtStart(
> +              flags, aState.mLastParticipant->mJustificationFlags, result);

Right now we set some flags for a pfd and then when we reach the next pfd, we overwrite the flags for the previous pfd.

I think it would be clearer to set the correct final values for mLastParticipant's end and this pfd's start here. When we reach the end of the line, set the last mParticipant's end-gap-count correctly. The important thing is to set the computed flags/gap-count for each end of the frane only once.

::: layout/generic/nsTextFrame.cpp
@@ +8874,5 @@
> +    flags |= TEXT_JUSTIFICATION_SPACE_AT_START;
> +    if (aFlags.mExtraSpaceAtStart) {
> +      flags |= TEXT_JUSTIFICATION_EXTRA_SPACE_AT_START;
> +    }
> +  }

This (and other code) would be simpler if instead of having JustificationFlags include 2 bools for each end, it included a single uint8_t for each end (whose value is 0, 1 or 2).

        Attachment #8514023 -
        Flags: review?(roc) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8514023 -
        Attachment is obsolete: true

        Attachment #8514767 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8514767 [details] [diff] [review]
patch

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

This is much much better, thank you!

::: layout/generic/nsLineLayout.cpp
@@ +613,5 @@
>    pfd->mBorderPadding = LogicalMargin(frameWM);
>    pfd->mOffsets = LogicalMargin(frameWM);
>  
> +  pfd->mJustificationInfo = JustificationInfo();
> +  pfd->mJustificationAssignment = JustificationAssignment();

We don't need these I think.

@@ +2367,4 @@
>        if (trimOutput.mDeltaWidth) {
>          pfd->mBounds.ISize(lineWM) -= trimOutput.mDeltaWidth;
>  
> +        if (!pfd->mNext) {

Why are you checking pfd->mNext here? I think there could be multiple textframes all being trimmed.

::: layout/generic/nsTextFrame.cpp
@@ +3148,5 @@
> +        mJustificationAssignments[i - mJustificationArrayStart];
> +      aSpacing[i - aStart].mBefore +=
> +        mJustificationSpacing * assign.mGapsAtStart;
> +      aSpacing[i - aStart].mAfter +=
> +        mJustificationSpacing * assign.mGapsAtEnd;

I'm worried that the total spacing added will not add up to the total justification spacing for the frame due to rounding errors.

I think you need to do what you do in nsLineLayout and keep a running total of how many gaps we've allocated, and compute the rounded-to-nscoord next/prev amount consumed and set the width of each gap to the difference between those values.

        Attachment #8514767 -
        Flags: review?(roc) → review-

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #92)
> Comment on attachment 8514767 [details] [diff] [review]
> patch
> 
> Review of attachment 8514767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is much much better, thank you!
> 
> ::: layout/generic/nsLineLayout.cpp
> @@ +613,5 @@
> >    pfd->mBorderPadding = LogicalMargin(frameWM);
> >    pfd->mOffsets = LogicalMargin(frameWM);
> >  
> > +  pfd->mJustificationInfo = JustificationInfo();
> > +  pfd->mJustificationAssignment = JustificationAssignment();
> 
> We don't need these I think.

I didn't think so either until I saw an unexpected failure on valgrind test. You can find the result here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a9d199b21d6

Anyway, I think it is safer to do so.

> ::: layout/generic/nsTextFrame.cpp
> @@ +3148,5 @@
> > +        mJustificationAssignments[i - mJustificationArrayStart];
> > +      aSpacing[i - aStart].mBefore +=
> > +        mJustificationSpacing * assign.mGapsAtStart;
> > +      aSpacing[i - aStart].mAfter +=
> > +        mJustificationSpacing * assign.mGapsAtEnd;
> 
> I'm worried that the total spacing added will not add up to the total
> justification spacing for the frame due to rounding errors.
> 
> I think you need to do what you do in nsLineLayout and keep a running total
> of how many gaps we've allocated, and compute the rounded-to-nscoord
> next/prev amount consumed and set the width of each gap to the difference
> between those values.

I'm just using what method was used in those places. I'm fine with changing it, though.

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #93)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #92)
> > Comment on attachment 8514767 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8514767 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> > ::: layout/generic/nsTextFrame.cpp
> > @@ +3148,5 @@
> > > +        mJustificationAssignments[i - mJustificationArrayStart];
> > > +      aSpacing[i - aStart].mBefore +=
> > > +        mJustificationSpacing * assign.mGapsAtStart;
> > > +      aSpacing[i - aStart].mAfter +=
> > > +        mJustificationSpacing * assign.mGapsAtEnd;
> > 
> > I'm worried that the total spacing added will not add up to the total
> > justification spacing for the frame due to rounding errors.
> > 
> > I think you need to do what you do in nsLineLayout and keep a running total
> > of how many gaps we've allocated, and compute the rounded-to-nscoord
> > next/prev amount consumed and set the width of each gap to the difference
> > between those values.
> 
> I'm just using what method was used in those places. I'm fine with changing
> it, though.

Well, mBefore and mAfter in Spacing are also gfxFloat, why do we need to use the running total method? I think the graphics part can process that cases correctly.
Flags: needinfo?(roc)

    
    

    
  
Spacing values should be rounded to the nearest nscoord. See definition of Spacing in gfxFont.h. That rounding introduces error and accumulated errors could lead to the total spacing added being different from the justification width allocated to the frame.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #95)
> Spacing values should be rounded to the nearest nscoord. See definition of
> Spacing in gfxFont.h. That rounding introduces error and accumulated errors
> could lead to the total spacing added being different from the justification
> width allocated to the frame.

OK, I'll fix it.
Flags: needinfo?(roc)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8514767 -
        Attachment is obsolete: true

        Attachment #8514847 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8514847 [details] [diff] [review]
patch

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

Great!

        Attachment #8514847 -
        Flags: review?(roc) → review+

    
    

    
  
Finally got the review granted, then I learn that the unrestricted union is not allowed in our code, so sad...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch, r=roc,masayuki (obsolete)
        — 
          — Splinter Review
      

    


  
Should be good now: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8d687b714a70

roc, you may want to check the code I change in this patch, but I think the changes should be fine.

        Attachment #8514847 -
        Attachment is obsolete: true

        Attachment #8515303 -
        Flags: feedback?(roc)

    
  

        Attachment #8515303 -
        Flags: review+

    
  
Keywords: checkin-needed

    
    

    
  
I guess we can move the JustificationApplicationState to MFBT, but I don't have a good idea about the name.

    
    

    
  


  
rebase to the latest m-c

        Attachment #8515303 -
        Attachment is obsolete: true

        Attachment #8515695 -
        Flags: review+

    
    

    
  


  
For justification-cjk-extension, in systems where the fonts lack the support of the Plane 2 characters, the two strings have different original width (see this attachment), hence the result is different. I don't know why it works fine previously, but the its current result is acceptable.

How can we fix this reftest? Is there any font covers all characters with a fixed width? Any idea?
Flags: needinfo?(jfkthame)

    
    

    
  


  
I have no idea about this regression on Android platform. Any suggestion, roc?
Flags: needinfo?(roc)

    
    

    
  
No. You probably should set up an Android debugging environment, it'll be worth it in the long run.
Flags: needinfo?(roc)

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #105)
> Created attachment 8517762 [details]
> comparison for justification-cjk-extension
> 
> For justification-cjk-extension, in systems where the fonts lack the support
> of the Plane 2 characters, the two strings have different original width
> (see this attachment), hence the result is different. I don't know why it
> works fine previously, but the its current result is acceptable.
> 
> How can we fix this reftest? Is there any font covers all characters with a
> fixed width? Any idea?

Not sure what range of characters you need to be fixed width, but http://sourceforge.net/adobe/csso9ntestfonts/wiki/Home/ might be of use.

    
    

    
  
(In reply to fantasai from comment #108)
> Not sure what range of characters you need to be fixed width, but
> http://sourceforge.net/adobe/csso9ntestfonts/wiki/Home/ might be of use.

Oh that's great! I found there is another font named Adobe Blank which should be enough in this case.
Flags: needinfo?(jfkthame)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for fixing reftests (obsolete)
        — 
          — Splinter Review
      

    


  
This patch fixes the reftests. But I'm not quite sure whether the changes are valid.

try server: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=20180cb3d3db

        Attachment #8517937 -
        Flags: review?(roc)

    
    

    
  
Gerv, is it OK for us to land in mozilla-central (for testing only) the Adobe font+license in https://bugzilla.mozilla.org/attachment.cgi?id=8517937 ?
Flags: needinfo?(gerv)

    
    

    
  
Comment on attachment 8517937 [details] [diff] [review]
patch for fixing reftests

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

It seems to me we might be best served by having our own test font (or modifying one of our existing test fonts) to have the same glyphs and metrics for a few Chinese characters, including the plane-2 characters, and having this test use that font and those characters. That would be simple and avoid any licensing issues.

::: layout/reftests/bugs/503399.html
@@ +5,5 @@
>      <style type="text/css">
>  
>          html,body {
>              color:black; background-color:white; font-size:16px;
> +            font-family: serif;

Do you know why this change is needed?

        Attachment #8517937 -
        Flags: review?(roc)

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #112)
> Comment on attachment 8517937 [details] [diff] [review]
> patch for fixing reftests
> 
> Review of attachment 8517937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems to me we might be best served by having our own test font (or
> modifying one of our existing test fonts) to have the same glyphs and
> metrics for a few Chinese characters, including the plane-2 characters, and
> having this test use that font and those characters. That would be simple
> and avoid any licensing issues.
> 
> ::: layout/reftests/bugs/503399.html
> @@ +5,5 @@
> >      <style type="text/css">
> >  
> >          html,body {
> >              color:black; background-color:white; font-size:16px;
> > +            font-family: serif;
> 
> Do you know why this change is needed?

TBH, I didn't know. But after you asked, I realize that it is a wrong fix which just simply make it skip the justification procedure for this test.

The reason of the failure on this test seems to be the minor difference of the distribution of the spacing. In fact, this minor difference exists in all our current version, even in all platform which the test passes. If you open attachment 8517789 [details] and zoom in, you can always observe the difference. It seems that the new algorithm slightly enlarges the difference in some platform in this testcase, which causes the failure. I have no idea how to fix it... Maybe we can try different position, and see if there is a place matches exactly in the two pages?

    
    

    
  
SIL OFL 1.1 is no problem at all. Go right ahead. If it's just test code, no need to adjust about:license.

Gerv
Flags: needinfo?(gerv)

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #105)
> Created attachment 8517762 [details]
> comparison for justification-cjk-extension
> 
> For justification-cjk-extension, in systems where the fonts lack the support
> of the Plane 2 characters, the two strings have different original width
> (see this attachment), hence the result is different. I don't know why it
> works fine previously, but the its current result is acceptable.
> 
> How can we fix this reftest? Is there any font covers all characters with a
> fixed width? Any idea?

Maybe you could use the small test font we already have in layout/reftest/fonts/gw432047.ttf? I believe this includes a CJK character in Plane 2 as well as a BMP one, so provided you use those specific characters in the test, it should avoid fallback/.notdef issues.

    
    

    
  
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #113)
> The reason of the failure on this test seems to be the minor difference of
> the distribution of the spacing. In fact, this minor difference exists in
> all our current version, even in all platform which the test passes. If you
> open attachment 8517789 [details] and zoom in, you can always observe the
> difference. It seems that the new algorithm slightly enlarges the difference
> in some platform in this testcase, which causes the failure. I have no idea
> how to fix it... Maybe we can try different position, and see if there is a
> place matches exactly in the two pages?

If you use a test font where the characters are just solid boxes, maybe we can fuzz the test results to account for small alignment differences.

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #116)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #113)
> > The reason of the failure on this test seems to be the minor difference of
> > the distribution of the spacing. In fact, this minor difference exists in
> > all our current version, even in all platform which the test passes. If you
> > open attachment 8517789 [details] and zoom in, you can always observe the
> > difference. It seems that the new algorithm slightly enlarges the difference
> > in some platform in this testcase, which causes the failure. I have no idea
> > how to fix it... Maybe we can try different position, and see if there is a
> > place matches exactly in the two pages?
> 
> If you use a test font where the characters are just solid boxes, maybe we
> can fuzz the test results to account for small alignment differences.

But the problem is that, in this test, we are checking the position of the caret. Using solid boxes and fuzzing it simply makes the test meaningless again, like my fix before.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for fixing reftests (obsolete)
        — 
          — Splinter Review
      

    


  
Should be good now: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aab42943095e

        Attachment #8517937 -
        Attachment is obsolete: true

        Attachment #8518653 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8518653 [details] [diff] [review]
patch for fixing reftests

Well, not so good. The failure for b2g is trivial which I can fix immediately. But it seems that the test for 503399 never makes effect on Windows, which now cause a unexpected failure. Should we simply skip it on windows?

        Attachment #8518653 -
        Flags: review?(roc)

    
    

    
  
Maybe you should add 503399 to test_reftests_with_caret.html?

    
    

    
  
OK I'll try.

    
    

    
  


  
Should be fine now: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6bf79646ee59
But it worth to mention that, in some of the platforms, test_reftests_with_caret is simply skipped. I'm not sure whether they can pass the tests once it is no longer skipped.

        Attachment #8518653 -
        Attachment is obsolete: true

        Attachment #8519619 -
        Flags: review?(roc)

    
  
Depends on: 1121350

    
  
Depends on: 1151684

    
  
Depends on: 1281457

          You need to log in
          before you can comment on or make changes to this bug.