bug682712-1.html: Fix and document this test

ASSIGNED
Assigned to

Status

()

Core
Editor
P3
normal
ASSIGNED
6 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
(In reply to Serge Gautherie (:sgautherie) from comment #26)
> Looking closer at bug 682712 and its test, I am puzzled by its code:
> 
> collapse(, 0): I think the current '1' is wrong (as in copy+paste typo).
> 
> Windows: after changing the value, this test passes on my local (as all the
> other "non-Windows" tests already do). You may not want to enable this one
> yet, though?
> 
> Submitted as https://hg.mozilla.org/try/rev/86a32644ef13 ...
> 
> NB:
> If I understood this code correctly, then the question would be why does
> this test succeeds currently on Linux and MacOSX...
> If I'm wrong, please explain and let's document better what this code is
> trying to do.


(In reply to Ehsan Akhgari [:ehsan] from comment #28)
> Hmm yeah, that definitely looks wrong.
(Assignee)

Updated

6 years ago
Blocks: 682712
(Assignee)

Comment 1

6 years ago
Created attachment 591016 [details] [diff] [review]
(Av1) bug682712-1.html: Fix and document this test

(In reply to Serge Gautherie (:sgautherie) from bug 717868 comment #26)

> Windows: after changing the value, this test passes on my local (as all the
> other "non-Windows" tests already do). You may not want to enable this one
> yet, though?
> 
> Submitted as https://hg.mozilla.org/try/rev/86a32644ef13 ...

Still failing on Windows FF tinderboxes, as expected.
Dropping that part.

> then the question would be why does
> this test succeeds currently on Linux and MacOSX...

This question stands...
Attachment #591016 - Flags: review?(ehsan)

Comment 2

6 years ago
Comment on attachment 591016 [details] [diff] [review]
(Av1) bug682712-1.html: Fix and document this test

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

::: layout/base/tests/bug682712-1-ref.html
@@ +10,5 @@
>          var iframe = document.querySelector("iframe");
>          var win = iframe.contentWindow;
>          var doc = iframe.contentDocument;
>  
> +        // Don't reframe the iframe.

No need for this comment, please remove it!

@@ +16,4 @@
>          setTimeout(function() {
>            synthesizeMouse(iframe, 10, 10, {});
>  
> +          // Set the caret and move it: after "f" char.

This comment is actually incorrect, please revert this change.

::: layout/base/tests/bug682712-1.html
@@ +10,5 @@
>          var iframe = document.querySelector("iframe");
>          var win = iframe.contentWindow;
>          var doc = iframe.contentDocument;
>  
> +        // Reframe the iframe: this was causing bug 682712.

The bug name is evident from the name of the file.  ;-)
Attachment #591016 - Flags: review?(ehsan) → review+
(Assignee)

Comment 3

6 years ago
Comment on attachment 591016 [details] [diff] [review]
(Av1) bug682712-1.html: Fix and document this test

(In reply to Ehsan Akhgari [:ehsan] from comment #2)

> No need for this comment, please remove it!

Agreed.

> > +          // Set the caret and move it: after "f" char.
> 
> This comment is actually incorrect, please revert this change.

I would really like to give more details about what ref and test are doing (differently).

Could you suggest a better comment than the existing one?
Like "Set the caret without moving it: after "f" char."?
(Though it puzzles me that it can be "without moving it" in both cases...)

> > +        // Reframe the iframe: this was causing bug 682712.
> 
> The bug name is evident from the name of the file.  ;-)

Yeah, what about "Reframe the iframe: this was the underlying cause of this bug."?
Attachment #591016 - Flags: feedback?(ehsan)

Comment 4

6 years ago
Comment on attachment 591016 [details] [diff] [review]
(Av1) bug682712-1.html: Fix and document this test

(In reply to Serge Gautherie (:sgautherie) from comment #3)
> Comment on attachment 591016 [details] [diff] [review]
> (Av1) bug682712-1.html: Fix and document this test
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #2)
> 
> > No need for this comment, please remove it!
> 
> Agreed.
> 
> > > +          // Set the caret and move it: after "f" char.
> > 
> > This comment is actually incorrect, please revert this change.
> 
> I would really like to give more details about what ref and test are doing
> (differently).
> 
> Could you suggest a better comment than the existing one?
> Like "Set the caret without moving it: after "f" char."?
> (Though it puzzles me that it can be "without moving it" in both cases...)

Well, the existing comment already describes what the reference is doing.

> > > +        // Reframe the iframe: this was causing bug 682712.
> > 
> > The bug name is evident from the name of the file.  ;-)
> 
> Yeah, what about "Reframe the iframe: this was the underlying cause of this
> bug."?

That would be evident if you read the bug.  In general I don't think that code comments should be a replacement for all of the information that exists in the bug.

An ideal patch here in my opinion is one which only replaces that 1 with 0 and doesn't change any of the existing comments.  ;-)
Attachment #591016 - Flags: feedback?(ehsan)
You need to log in before you can comment on or make changes to this bug.