Last Comment Bug 517882 - Implement -moz-tab-size
: Implement -moz-tab-size
Status: VERIFIED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
: 430432 (view as bug list)
Depends on:
Blocks: 295506
  Show dependency treegraph
 
Reported: 2009-09-21 06:59 PDT by Geoff Lankow (:darktrojan)
Modified: 2010-06-22 10:22 PDT (History)
12 users (show)
dbaron: wanted1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (12.24 KB, patch)
2009-09-21 06:59 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
patch v2 (12.21 KB, patch)
2009-09-21 18:01 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
patch v3 (14.30 KB, patch)
2009-09-22 04:50 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
patch v4 (19.04 KB, patch)
2009-09-22 17:38 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
patch v5 (21.12 KB, patch)
2009-09-24 03:18 PDT, Geoff Lankow (:darktrojan)
roc: review+
Details | Diff | Review
patch v6 (22.91 KB, patch)
2009-09-24 17:24 PDT, Geoff Lankow (:darktrojan)
dbaron: review+
Details | Diff | Review
patch v7 (23.44 KB, patch)
2009-09-25 00:46 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
follow-up build-warning fix: reorder nsStyleText constructor's initializer list (878 bytes, patch)
2009-10-14 17:48 PDT, Daniel Holbert [:dholbert]
dbaron: review+
Details | Diff | Review

Description Geoff Lankow (:darktrojan) 2009-09-21 06:59:48 PDT
Created attachment 401843 [details] [diff] [review]
patch v1

Variable tab width for use in things such as view-source.

I really don't expect that this patch is up to scratch, but it does appear to work.
Comment 1 Markus Stange [:mstange] 2009-09-21 16:58:41 PDT
If you want this patch to be reviewed, try requesting review from dbaron.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-09-21 17:07:48 PDT
Yes, dbaron will need to review this.

The patch looks pretty good to me!

+  } else if (eCSSUnit_Null == textData.mTabSize.GetUnit()) {

Shouldn't this be eCSSUnit_Initial?

We'll want at least one reftest here. Should be pretty easy, e.g.

<div style="white-space:pre; -moz-tab-size:4;">A&#9;X</div>
vs
<div style="white-space:pre;">A   X</div>
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-09-21 17:31:21 PDT
Hmm, you might need mTabSize to be PRInt32. As-is you can get overflow here:
+    text->mTabSize = textData.mTabSize.GetIntValue();
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-09-21 17:49:36 PDT
Before requesting review from me, you should make appropriate changes to layout/style/test/property_database.js to add the new property, and then make sure that the tests in that directory pass.  You can run them with something like:

cd $OBJDIR/_tests/testing/mochitest/
python ./runtests.py --test-path=layout/style --autorun

Also see https://developer.mozilla.org/en/Mozilla_automated_testing#Mochitest
Comment 5 Geoff Lankow (:darktrojan) 2009-09-21 18:01:50 PDT
Created attachment 401996 [details] [diff] [review]
patch v2
Comment 6 Geoff Lankow (:darktrojan) 2009-09-21 18:06:24 PDT
(In reply to comment #3)
> Hmm, you might need mTabSize to be PRInt32. As-is you can get overflow here:
> +    text->mTabSize = textData.mTabSize.GetIntValue();

Not quite sure what I was thinking when I wrote that. Monkey see, monkey do, I guess.

(In reply to comment #4)
> Before requesting review from me, you should make appropriate changes to
> layout/style/test/property_database.js to add the new property, and then make
> sure that the tests in that directory pass.  You can run them with something
> like:
> 
> cd $OBJDIR/_tests/testing/mochitest/
> python ./runtests.py --test-path=layout/style --autorun
> 
> Also see https://developer.mozilla.org/en/Mozilla_automated_testing#Mochitest

Yup, I'll get on to that.
Comment 7 Geoff Lankow (:darktrojan) 2009-09-22 04:50:59 PDT
Created attachment 402065 [details] [diff] [review]
patch v3

Mochitests passed
Reftests when I find time
Comment 8 Geoff Lankow (:darktrojan) 2009-09-22 17:38:11 PDT
Created attachment 402238 [details] [diff] [review]
patch v4

Now with reftests
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-09-23 22:53:52 PDT
nsCSSPropList.h:

+    CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE,

I think this should not apply to :first-letter and :first-line.  (I think
the closest analogous property is 'white-space'.)

nsComputedDOMStyle.cpp:

+  val->SetNumber (GetStyleText()->mTabSize);

No space between SetNumber and (.

>+    COMPUTED_STYLE_MAP_ENTRY_LAYOUT(_moz_tab_size,          MozTabSize),

Drop the "_LAYOUT"; that's for entries that require layout information
in the method implementation.

nsRuleNode.cpp:

+  switch (textData.mTabSize.GetUnit()) {
+    case eCSSUnit_Initial:
+      text->mTabSize = 8;
+      break;
+    case eCSSUnit_Inherit:
+      text->mTabSize = parentText->mTabSize;
+      break;
+    case eCSSUnit_Integer:
+      text->mTabSize = textData.mTabSize.GetIntValue();
+      break;
+  }

Instead of doing this, you should use SetDiscrete.  (SetDiscrete will
turn out useful when we implement additional generic values.)

nsTextFrameThebes.cpp:

>+  const nsStyleText* textStyle = aLineContainer->GetStyleText();

This makes it seem like you're not supporting tab-width on inline
elements, only on blocks.  Is that intended?  It's inconsistent with
'white-space', which seems like the most related property.

In order to support it there, I suspect you might need to do something
to ensure that we don't continue text runs across changes in the
'-moz-tab-size' property.  But roc would know for sure.

>+  return textStyle->mTabSize*spaceWidthAppUnits;

I think this would be more readable with space on either side of the *.

roc needs to review the changes to nsTextFrameThebes.cpp (and look at
the above comments)
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-09-23 22:58:24 PDT
Another question for roc:  does the result of the calculation that currently uses 8 for tab width end up in the text run cache, such that it could get re-used for another text run with different tab-width?
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-09-23 23:32:31 PDT
(In reply to comment #9)
> This makes it seem like you're not supporting tab-width on inline
> elements, only on blocks.  Is that intended?  It's inconsistent with
> 'white-space', which seems like the most related property.

Yes, it may as well work on inlines, it's just a matter of getting the tab size from the text frame's style context. Guess we should have a test for that. Otherwise looks good.

> In order to support it there, I suspect you might need to do something
> to ensure that we don't continue text runs across changes in the
> '-moz-tab-size' property.  But roc would know for sure.

You don't. Spacing information is not stored in the textrun.

(In reply to comment #10)
> Another question for roc:  does the result of the calculation that currently
> uses 8 for tab width end up in the text run cache, such that it could get
> re-used for another text run with different tab-width?

No.
Comment 12 Geoff Lankow (:darktrojan) 2009-09-24 03:18:14 PDT
Created attachment 402559 [details] [diff] [review]
patch v5

I'm not sure I understood everything that was said as most of this code is completely new to me, but I think I've covered it all.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-09-24 03:25:56 PDT
Comment on attachment 402559 [details] [diff] [review]
patch v5

Good enough for me.

A reftest that tested -moz-tab-size on spans might be worth having too. E.g.
<pre><span style="-moz-tab-size:4">&#9;</span>X</pre>
Comment 14 Geoff Lankow (:darktrojan) 2009-09-24 03:28:00 PDT
Comment on attachment 402559 [details] [diff] [review]
patch v5

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp

>+  // tab-size: integer, inherit
>+  SetDiscrete(textData.mTabSize, text->mTabSize, canStoreInRuleTree,
>+              SETDSC_NORMAL | SETDSC_INTEGER, parentText->mTabSize,
>+              NS_STYLE_TABSIZE_NORMAL, 0, 0, 0, 0);

On second thoughts (now that I've gone and uploaded the patch... typical):

That 4th argument should be just SETDSC_INTEGER.
The constant would be better named NS_STYLE_TABSIZE_INITIAL.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-09-24 12:25:34 PDT
Comment on attachment 402559 [details] [diff] [review]
patch v5

> // See nsStyleText
>+#define NS_STYLE_TABSIZE_NORMAL                 8

Yeah, I agree this should be INITIAL rather than NORMAL.

>+  // tab-size: integer, inherit
>+  SetDiscrete(textData.mTabSize, text->mTabSize, canStoreInRuleTree,
>+              SETDSC_NORMAL | SETDSC_INTEGER, parentText->mTabSize,
>+              NS_STYLE_TABSIZE_NORMAL, 0, 0, 0, 0);

You don't need the SETDSC_NORMAL here ; SETDSC_INTEGER alone should be sufficient.  SETDSC_NORMAL would be for a property that accepts 'normal' as a value.

Fix those two and add some reftests for -moz-tab-size on inlines, and this looks good.
Comment 16 Geoff Lankow (:darktrojan) 2009-09-24 17:24:37 PDT
Created attachment 402722 [details] [diff] [review]
patch v6
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-09-24 23:29:43 PDT
Comment on attachment 402722 [details] [diff] [review]
patch v6

I just noticed one other thing:

layout/reftests/reftest.list needs an "include tab-size/reftest.list", or the reftests you added won't get run.

r=dbaron with that fixed.

I'll probably edit that myself shortly and then push this to try server (https://wiki.mozilla.org/Build:TryServer) to make sure the tests are all fine.
Comment 18 Geoff Lankow (:darktrojan) 2009-09-25 00:46:54 PDT
Created attachment 402787 [details] [diff] [review]
patch v7
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-09-26 13:28:10 PDT
Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/f4f374e783cc

Good work on the patch.
Comment 20 Christopher Blizzard (:blizzard) 2009-10-07 10:48:27 PDT
For docs:

http://lists.w3.org/Archives/Public/www-style/2008Dec/0009.html
Comment 21 Daniel Holbert [:dholbert] 2009-10-14 17:48:31 PDT
Created attachment 406373 [details] [diff] [review]
follow-up build-warning fix: reorder nsStyleText constructor's initializer list

This checkin introduced this build warning (on Linux):

> nsStyleStruct.h:1076: warning: ‘nsStyleText::mTextShadow’ will be initialized after
> nsStyleStruct.h:1069: warning:   ‘PRInt32 nsStyleText::mTabSize’
> nsStyleStruct.cpp:2359: warning:   when initialized here

Attached follow-up patch fixes this by reordering the constructor's initialization list to put mTabSize between mWordWrap and mLetterSpacing (to match the order used in the nsStyleText class declaration).
Comment 22 Daniel Holbert [:dholbert] 2009-10-14 19:43:13 PDT
build-warning fix landed: http://hg.mozilla.org/mozilla-central/rev/cb01e7897aad
Comment 23 Nickolay_Ponomarev 2010-02-02 06:06:50 PST
*** Bug 430432 has been marked as a duplicate of this bug. ***
Comment 25 :Ms2ger 2010-06-22 10:18:44 PDT
Verifying.
Comment 26 Eric Shepherd [:sheppy] 2010-06-22 10:22:09 PDT
Added it to the Firefox 4 for developers page as well.

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