Bug 119063 – HistoryController: setting the pageScaleFactor and scrollPoint properly

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore Misc.

[Thiago de Barros Lacerda]

HistoryController: setting the pageScaleFactor and scrollPoint properly

[Thiago de Barros Lacerda]

Created attachment 207409 [details]
Patch

[Jesus Sanchez-Palencia]

Comment on attachment 207409 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207409&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You should either remove the entire line or state "No new tests needed because of foo....". Leaving the (OOPS!) here is wrong ;) .

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 207409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207409&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests (OOPS!).
> 
> You should either remove the entire line or state "No new tests needed because of foo....". Leaving the (OOPS!) here is wrong ;) .

Correction - You should either add tests, explain what tests cover this already, or explain why tests are not possible.

It's not okay to just remove this line without explanation.

[Jesus Sanchez-Palencia]

(In reply to comment #3)
> It's not okay to just remove this line without explanation.

Well, sure, crystal clear everything I said is assuming if you left a line saying "NO NEW TESTS" you know what you are doing, therefore you are not adding new tests due to a reason.

Needless to say adding tests is usually better...

[Thiago de Barros Lacerda]

There are not tests for HistoryController, even with the old behaviour.
Do you think it is worth adding test for that?

[Thiago de Barros Lacerda]

Created attachment 207457 [details]
Patch

[Brady Eidson]

(In reply to comment #5)
> There are not tests for HistoryController, even with the old behaviour.
> Do you think it is worth adding test for that?

Is it testable?

[Thiago de Barros Lacerda]

(In reply to comment #7)
> (In reply to comment #5)
> > There are not tests for HistoryController, even with the old behaviour.
> > Do you think it is worth adding test for that?
> 
> Is it testable?

I can think now of a simple test, creating a HistoryController instance and 2 HistoryItems, with different scales and scroll points, and play around going back and forward with them. I think it is enough for testing it. What do you think?

[Brady Eidson]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > There are not tests for HistoryController, even with the old behaviour.
> > > Do you think it is worth adding test for that?
> > 
> > Is it testable?
> 
> I can think now of a simple test, creating a HistoryController instance and 2 HistoryItems, with different scales and scroll points, and play around going back and forward with them. I think it is enough for testing it. What do you think?

How does that translate to a LayoutTest?

[Thiago de Barros Lacerda]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > There are not tests for HistoryController, even with the old behaviour.
> > > > Do you think it is worth adding test for that?
> > > 
> > > Is it testable?
> > 
> > I can think now of a simple test, creating a HistoryController instance and 2 HistoryItems, with different scales and scroll points, and play around going back and forward with them. I think it is enough for testing it. What do you think?
> 
> How does that translate to a LayoutTest?

I have been trying for the last days to simulate the behaviour I'm facing with the UIProcess running, to a LayoutTest... But no success... the code paths are _very_ different.

[Thiago de Barros Lacerda]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > There are not tests for HistoryController, even with the old behaviour.
> > > > Do you think it is worth adding test for that?
> > > 
> > > Is it testable?
> > 
> > I can think now of a simple test, creating a HistoryController instance and 2 HistoryItems, with different scales and scroll points, and play around going back and forward with them. I think it is enough for testing it. What do you think?
> 
> How does that translate to a LayoutTest?

Brady, I couldn't make a LayoutTest on that. But as you can see this patch is very reasonable and we need it in Nix port. Additionally I can't see how it can add any problems to other ports.

Regards

[Thiago de Barros Lacerda]

Created attachment 210657 [details]
Patch

[Thiago de Barros Lacerda]

Rebased on master

[Darin Adler]

Comment on attachment 210657 [details]
Patch

Can you make a regression test for this, please?

[Sam Weinig]

Comment on attachment 210657 [details]
Patch

As many have noted, this needs a test.

[Thiago de Barros Lacerda]

*** This bug has been marked as a duplicate of bug 126022 ***