Skip to content

Test PR#1

Closed
foolip wants to merge 6 commits intomasterfrom
test-branch
Closed

Test PR#1
foolip wants to merge 6 commits intomasterfrom
test-branch

Conversation

@foolip
Copy link
Owner

@foolip foolip commented Jul 24, 2018

No description provided.

@foolip foolip closed this Aug 7, 2018
@foolip foolip deleted the test-branch branch October 10, 2018 09:06
foolip pushed a commit that referenced this pull request Apr 1, 2019
chromedriver doesn't allow changing Object.prototype to add enumerable
properties, but this test requires setting some values on
Object.prototype.  When Object.prototype.a is set to:

  {b: {c: 'on proto'}}

chromedriver fails with:

    JavascriptErrorException: javascript error (500): Maximum call stack size exceeded
      (Session info: chrome=72.0.3626.121)

    Remote-end stacktrace:

    #0 0x563ff3a32a59 <unknown>
    #1 0x563ff39cb7f3 <unknown>
    #2 0x563ff38fcd7c <unknown>
    #3 0x563ff38ff78c <unknown>
    #4 0x563ff38ff5f7 <unknown>
    #5 0x563ff38ffbe7 <unknown>
    #6 0x563ff38fff1b <unknown>
    #7 0x563ff38a3f7a <unknown>
    #8 0x563ff3899bf2 <unknown>
    #9 0x563ff38a37b7 <unknown>
    #10 0x563ff3899ac3 <unknown>
    #11 0x563ff38782d2 <unknown>
    #12 0x563ff3879112 <unknown>
    #13 0x563ff39fe865 <unknown>
    #14 0x563ff39ff32b <unknown>
    #15 0x563ff39ff70c <unknown>
    #16 0x563ff39d940a <unknown>
    #17 0x563ff39ff997 <unknown>
    #18 0x563ff39e9947 <unknown>
    #19 0x563ff3a1a800 <unknown>
    #20 0x563ff3a3c8be <unknown>
    #21 0x7f3bf4545494 start_thread
    #22 0x7f3bf2d58a8f clone

    Ran 1 tests finished in 2.0 seconds.
      • 0 ran as expected. 0 tests skipped.
      • 1 tests had errors unexpectedly

Work around this problem by cleaning up the test environment so
Object.prototype no longer has the override by the time chromedriver
tries to inspect the test result.

While here, fix the other tests to use the t.add_cleanup() function
so they'll cleanup their test environment in case they exit in
some other way besides reaching t.done().

The underlying chromedriver issue is tracked upstream at
https://crbug.com/chromedriver/2555.

Bug: 934844
Change-Id: Id1b4ab2a908bfbc001e2a2d045eeec3ef01c24d9
foolip pushed a commit that referenced this pull request Apr 1, 2019
We used to commit navigation after receiving the first byte of
document response. This CL moves commit earlier, synchronously done
from CommitNavigation call. The change should not be web-observable,
but some internal assumptions may have been affected.

Test changes:
- ReplacingDocumentLoaderFiresLoadEvent was testing the old behavior,
  which is not applicable anymore.
- MultiChunkWithReentrancy now uses a different method to trigger
  reentrancy (pdf plugin), since we no longer commit after first byte.
- backdrop-object.html and anchor-change-href.svg relied on test finishing
  late enough, now they wait for onload to eliminate a race.
- use-property-synchronization-crash.html now reports an error message
  synchronously and therefore has JS stack and a line number.
- setting-allowpaymentrequest-timing.https.sub.html has a race as
  explained here [1], and now fails even without site isolation.

This corresponds to the step 8.b from the doc linked to the bug.

Difference from attempt #1 (https://chromium-review.googlesource.com/c/1399447):
- PluginDocumentParser and MediaDocumentParser early return if not parsing
  before accessing GetDocument. This is because DocumentLoader calls Finish()
  even after parser was stopped/detached. For example in Document::Abort
  we cancel parsing, but committed DocumentLoader might be still receiving data.
  We should ideally clean up all calls into parser, there are numerous TODOs
  for that.
- pageload-image-in-popup.html relies on small image being parsed in the same
  task as navigation commit. Using onload seems to fix the issue.
- touch-handler-iframe-plugin-assert.js hopes that onload for about:blank
  happens after test has finished, which is racy now.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=819800#c6

Bug: 855189, 937639, 836242, 937358
Change-Id: I65048a27e6d249a608d4eb61e5c882292386026e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1506663
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#639992}
foolip pushed a commit that referenced this pull request Apr 1, 2019
All tests pass, and crashes no longer happen. I believe
that code will not longer crash, but there might be
futher instances of incorrect positioning.

Fix #1
LayoutDescendantCandidates did not sweep newly discovered
candidates. This was done
manually once inside NGOutOfFlowLayoutPart::Run, and
sweep was not performed for LayoutDescendantCandidates
found in Legacy. Fix is to make LayoutDescendantCandidates
perform sweep instead.

Fix #2
fix #1 exposed a bug where duplicate fragments were generated
for a single layout object. This happened when NG was generating
fragments not inside ContainingBlock. Fix one instance of this
inside NGOutOfFlowLayoutPart::IsContainingBlockForDescendant
by making sure that OOF with inline containers are only positioned
inside its ContainingBlock()

Fix #3
NGOutOfFlowLayoutPart::LayoutDescendant offset adjustment.

Bug: 935805
Change-Id: I9f7ebbc7223f40fbbf6ba3739d9385bfd59e3641
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1517093
Commit-Queue: Aleks Totic <atotic@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#641628}
foolip pushed a commit that referenced this pull request Jun 2, 2021
Currently, the CheckCanStartAnimationOnCompositor is called twice
for composite background-color animation, once during paint time
and once during the compositing stage. The reason is that we
need the decision during paint and compositing consistent. That
is, if the paint stage says the background color must be paint
on the main thread, then compositing stage has to agree with
that, and vice versa. However, this is dangerous because between
the paint and compositing stage, things could change, especially
the PaintArtifactCompositor, which is used by the
CheckCanStartAnimationOnCompositor. For example, it could
happen that at paint time we have not produced / updated the
property nodes for the current frame and we can make decision
based on what was composited on the previous frame. Then
at Precommit we have potentially updated / added / removed
property tree nodes. In this case, the return value of
CheckCanStartAnimationOnCompositor can be different, as a
result, the background color animation won't run correctly.

The reason we needed to know whether the animation could be
composited here is that we didn't have a way to paint
the background color off the main thread. More specifically,
the BackgroundColorPaintWorklet::Paint() function can paint the
background color only if the animation is running on the
compositor thread.

This CL makes following changes:
1. Make the BackgroundColorPaintWorklet::Paint() have the
ability to paint the background color even if the animation is
running on the main thread. The function needs two things:
the current progress of the animation and the artifacts about
the animation. So all we need is just getting the progress
when the animation is running on the main thread.
2. With #1 being done, we no longer need to call the
CheckCanStartAnimationOnCompositor during the paint step.
As a result, whether or not the animation can be running on
the compositor thread is solely the decision during the
compositing stage. This is much safer than the current code,
because we no longer need to make a compositing decision during
the paint stage.

We don't need to add any new tests because we already have
sufficient layout tests for background color animation being
run on the compositor as well as on the main thread. As long
as all tests pass, this should be safe.

The main benefit of this change is that the code is now more
robust, meaning that we don't need to worry about the decision
made by the paint and compositing stage being different.
This change is also a performance win because we no longer
need to call the CheckCanStartAnimationOnCompositor twice.

Bug: 1185272, 1182261
Change-Id: Ie072714fd1d05e6537e05cad45ad1da99e20125b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2740697
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#863622}
foolip pushed a commit that referenced this pull request Jun 2, 2021
…eb-platform-tests#28617)

Subresource Web Bundles.

The problem is: when Web Bundle fetching fails due to a network error,
Subresource fetch doesn't fail forever.
One such case (subresource-loading-cors-error test) was
timing out previously but passes successfully with this change.

This CL also adds 2 WPT tests:
1. subresource-loading-network-error.https.tentative.sub.html
2. subresource-loading-web-bundle-fetch-failed.https.tentative.html

Test #1 is a scenario with a different network error than the CORS
one, but with the same issue of subresource fetching timing out
without the change. It passes successfully after the change.

Test #2 is a scenario with a Web bundle not found error, which is
not directly influenced by the code added in this CL, but it expands
the test coverage which was found to be lacking the error cases before.

Bug: 1168449

Change-Id: Ia3abb967e36274becc86e317bc51b1272d3ae679
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2826001
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Miras Myrzakerey <myrzakereyms@google.com>
Cr-Commit-Position: refs/heads/master@{#875532}

Co-authored-by: Miras Myrzakerey <myrzakereyms@google.com>
foolip pushed a commit that referenced this pull request Jun 10, 2021
1. Only process ChildrenChanged() for the included root of a change.
For example, if a <div id="root" style="display:none"> will be
included because it is a potential relation target. If descendants
change, the only ChildrenChanged() necessary to process is on #root.
2. Share common code for detaching a node and queuing up the appropriate
children changes. This simplifies ProcessInvalidatedObjects()
by removing one of the inner loops, and enables a follow-up CL to
remove the outer loop as well.

#1 results in a massive speedup for display none toggles. In
combination with other recent changes in
DetachAndRemoveFromChildrenOfAncestors(), is 7x faster for
many-nodes-toggle-display-none in perf_tests . This change alone
accounts for about half of the overall improvement.

Follow-ups:
- Restore lifecycle check by processing deferred children changes via
nodes_with_pending_children_changed_ and not queuing via the
traditional mechanism. While doing this, look for opportunities to
consolidate more children changed events.
- Remove outer loop from ProcessInvalidatedObjects().

Bug: None
Change-Id: I80466fda792cd0ca6dd051065a42ba702e4cc8b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2946971
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#891343}
foolip pushed a commit that referenced this pull request Sep 29, 2021
1. Use GetWithoutInvalidation() instead of Get() in DCHECKs.
We should never call Get() inside of a DCHECK(), because this can
lead to a different code path depending on whether DCHECKs are enabled.

2. Get() should not cause immediate side effects. At most, it should
queue up an invalidation for later processing.

Fixing #1 and #2 were required in order to get past a first set of
errors introduced by the new test.

3. The actual fix -- avoid infinite loop by calling a special
new SlotAssignmentWillChange(), rather than ChildrenChanged(),
where a minimal GetWithoutInvalidation() is called that does not
lead to IsShadowContentRelevantForAccessibility() => FirstChild() =>
RecalcAssignedNodes() => ChildrenChanged() ... (infinite loop).

A simpler potential fix is in CL:2965317 but requires more
research. It's also mentioned in a TODO comment.

Bug: 1219311
Change-Id: Iafaa289f241a851404ce352715d2970172a2e5f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2961158
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#892778}
foolip pushed a commit that referenced this pull request Sep 29, 2021
Relative offsets should be applied after fragmentation. Since we perform
layout for OOFs once they reach the fragmentation context root (if
applicable), we fail to apply any relative offsets at the correct time
in the case of inline containing blocks. See CL:2851595 for how this
was handled for the non-inline case.

The changes required to accomplish this for inline containing blocks
include:

1. We currently store an accumulated relative offset in
NGContainingBlock inside the OOF node, which is any relative offset from
the containing block to the fragmentation context root. We also need to
store an accumulated relative offset from the inline container to the
containing block in order to properly apply all relative offsets at the
time of fragmentation.

A new struct, NGInlineContainer, was added to the OOF node to hold the
inline container object and the accumulated relative offset to the
containing block.

2. A relative offset was also added to the InlineContainingBlockGeometry
struct so that we have access to the relative offset from #1 when
creating the ContainingBlockInfo for the inline container.

3. The way that relative offsets are applied to inlines, it didn't seem
straightforward to separate the relative offset from the normal
offset, like we had in CL:2851595. Instead, store the relative offset
for the inline and subtract it out from the OOF static position once
it reaches the containing block, and subtract it from the containing
block rect offset when determining the ContainingBlockInfo for the
inline container.

4. Store the total relative offset (from the inline container to the
fragmentation context root) in ContainingBlockInfo. This relative
offset will then be applied after fragmentation is complete for the OOF
as a result of CL:2851595.

Bug: 1079031
Change-Id: I7198fec4c01e05ca54c51b2f215569b75b0b869e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2995308
Commit-Queue: Alison Maher <almaher@microsoft.com>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#902060}
foolip pushed a commit that referenced this pull request Oct 7, 2021
This patch adds a new produceCropId() API to mediaDevices.

This API is called with a DIV or IFRAME element, and adds a new
base::UnguessableToken value to that element's rare data structure.

This token value will be used in followup patches in order to keep track
of an element's location in the page and viewport.

Based on the following design document:
https://docs.google.com/document/d/1dULARMnMZggfWqa_Ti_GrINRNYXGIli3XK9brzAKEV8/

Bug: 1247761
Change-Id: I01cd67e2d4e3dfa7a86289f876e48c8b55095d0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173396
Commit-Queue: Jordan Bayles <jophba@chromium.org>
Reviewed-by: Elad Alon <eladalon@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#925544}
foolip pushed a commit that referenced this pull request Jan 20, 2022
…eVisibilityKeeper::PrepareToSplitBlockElement()` before splitting a text node

It does the following things when caret is collapsed in a text node in a `<p>`
or `<div>` element.

1. Split the text node containing caret to insert `<br>` element
2. Insert `<br>` element after it
3. Split ancestor elements which inclusive descendants of the `<p>` or `<div>`
4. Delete the `<br>` element if unnecessary from the left paragraph

#3 and #4 are performed by `HTMLEditor::SplitParagraph()` and it calls
`WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement()` correctly before
splitting the block.  However, in the case (caret is at middle of a text node),
the text has already been split to 2 nodes because of #1.  Therefore, it fails
to handle to keep the white-space visibility.

So that I believe that the root cause of this bug is, the method does much
complicated things which are required, and doing the redundant things will
eat memory space due to undo transactions.  However, for now, I'd like to fix
this with a simple patch which just call the preparation method before splitting
the text node because I'd like to uplift this if it'd be approved (Note that
this is not a recent regression, the root cause was created by bug 92686 which
was fixed in 17 years ago:
<https://searchfox.org/mozilla-central/commit/2e66280faef73e9be218e00758d4eb738395ac83>,
but must be annoying bug for users who see this frequently).

The new WPTs are pass in Chrome.

Differential Revision: https://phabricator.services.mozilla.com/D130950

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1740416
gecko-commit: 73567f6c2bcfa078836a36760498bb11747561dd
gecko-reviewers: m_kato, smaug
foolip pushed a commit that referenced this pull request Feb 25, 2022
By adding new exhaustive tests under ordering/, it was revealed that the ordering between navigatesuccess/navigateerror and the committed/finished promises was not always consistent:

1. Simply adding a currentchange event handler would cause microtasks to run during commit, which changed some ordering.

2. Calling transitionWhile() would take us from the zero-promise case to the 1+-promise case in ScriptPromise::All(). As the new comment explains, both the spec and implementation have an observably-different fast path for the 0-promise case which caused changes in ordering.

In the course of fixing this, I found out that the did_finish_before_commit_ code in app_history_api_navigation.{h,cc} was actually not a mitigation for the case it stated, where promises passed to transitionWhile() would settle faster than the browser-process roundtrip for same-document traversals. That is in fact impossible, since we only fire the navigate event after the browser-process roundtrip has completed. Instead, they were a mitigation for (1).

This commit then ensures consistent ordering, tested with new rather-exhaustive tests, in the following manner:

* We move the firing of currentchange to before resolving the committed promise. This eliminates (1) and allows us to delete the did_finish_before_commit_ tracking.

* We always ensure we pass 1+ promises to ScriptPromise::All(). This eliminates (2).

A consequence of this is that we are now more likely to get rejected finished promises, in cases like

    await appHistory.navigate("#1").committed;
    await appHistory.navigate("#2").committed;

Before, the finished promise for the #1 navigation would go through the fast path per (2), and fulfill before the navigation to #2 canceled it. Now that does not happen, so code like the above will give an unhandled promise rejection for #1's finished promise.

To avoid this, we unconditionally mark finished promises as handled. This follows some web platform precedent, e.g. stream closed promises, where the promise is one of several information channels (in this case the developer might also find out via the AbortSignal or the navigateerror event). We do *not* do this for the committed promise though, as if a commit fails, that's probably something more deeply wrong, and cannot be ignored.

All of these changes will require spec updates.

For the tests, we introduce a new ordering/ directory which contains cross-cutting ordering tests, and we consolidate a few tests into the newly-introduced variant-driven exhaustive ones. A couple of other tests were affected by these changes too or fixed as a drive-by.

Change-Id: I8a50ca28d009e0a8a2c94331cd17f29b0a8dc463
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3405377
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#963772}
foolip pushed a commit that referenced this pull request Jul 5, 2022
Now, popups will follow this process when showing/hiding:

showPopup():
 1. Move the popup to the top layer, and remove the UA display:none
    style.
 2. Update style. (Transition initial style can be specified in this
    state.)
 3. Set the :top-layer pseudo class.
 4. Update style. (Animations/transitions happen here.)

hidePopup():
 1. Capture any already-running animations via getAnimations().
 2. Remove the :top-layer pseudo class.
 3. Update style. (Animations/transitions start here.)
 4. If the hidePopup() call is not due to a "force out" situation,
    getAnimations() again, remove any from step #1, and then wait here
    until all of them finish or are cancelled.
 4. Remove the popup from the top layer, and add the UA display:none
    style.
 5. Update style.

See this issue for more details:
  openui/open-ui#335

Bug: 1307772
Change-Id: Ia20eb6e9533c1a0b1029ca1279d42fe2648300af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3688871
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1014235}
foolip pushed a commit that referenced this pull request Jul 5, 2022
See [1] for the origin of this change, which makes it possible to
trigger pop up hide animations from within the `hide` event handler.
For example:

  popup.addEventListener('hide', () => {
    popup.animate({
      transform: 'translateY(-50px)',
      opacity: 0,
    }, 200);
  });

To accomplish that, the hide process now looks like this:

 1. Capture any already-running animations via getAnimations(),
    including animations on descendent elements.
 2. Remove the :top-layer pseudo class.
 3. Fire the 'hide' event.
 4. If the hidePopup() call is *not* the result of the pop-up being
    "forced out" of the top layer, e.g. by a modal dialog or fullscreen
    element:
   a. Restore focus to the previously-focused element.
   b. Update style. (Animations/transitions start here.)
   c. Call getAnimations() again, remove any from step #1, and then wait
      until all of them finish or are cancelled.
 5. Remove the pop-up from the top layer, and add the UA display:none
    style.
 6. Update style.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/3688871/9/third_party/blink/renderer/core/dom/element.cc#2660

Bug: 1307772

Change-Id: I910535b13cfc3c8f8498ed64dae73caa75dd7317
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708419
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Commit-Queue: Robert Flack <flackr@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1018685}
foolip pushed a commit that referenced this pull request Dec 21, 2022
******************************************************************
*** SHERIFFS: please don't revert this CL if it causes web_tests
              to flake/fail. If that happens, the cause is a bad
              test. Please mark that test as flaky/fail in
              TestExpectations, with a new crbug. Please block the
              new bug against crbug.com/1395228. Thanks!
******************************************************************

Prior to this CL, a test like this:

```
<script>
window.onload = () => {
  test((t) => { ... }, 'test 1');
  test((t) => { ... }, 'test 2');
  test((t) => { ... }, 'test 3');
};
</script>
```

would not run anything after test #1. The issue is that the testharness
immediately adds a window load handler that marks `all_loaded = true`,
and that ends the tests as soon as the first result from the first test
is processed. (The test runner waits for the first test because
`Tests.prototype.all_done()` also waits until `this.tests.length > 0`.)
There were various mitigating corner cases, such as if you started
the list of tests with a promise_test(), that would increment a
counter that kept the rest of the tests alive. Etc.

With this CL, the testharness-added window.onload handler runs a
setTimeout(0), so that `all_loaded` is only set to true after all of
the tests are loaded by any window.onload handler.

This exposed a few tests that should have been failing but were
masked by the lack of test coverage - bugs have been filed for
those. Also, several tests that were working around this via various
means are also cleaned up in this CL. I'm sure there are more of
those.

Bug: 1395228,1395226,1307772
Change-Id: I6f12b5922186af4e1e06808ad23b47ceac68559c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4074305
Reviewed-by: Weizhong Xia <weizhong@google.com>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1081558}
foolip added a commit that referenced this pull request Dec 21, 2022
This reverts commit 4a03c6c459fdbf11976a424aa02a1d094484134c.

Reason for revert:

This has caused tests in upstream WPT to fail, blocking unrelated PRs. It was still possible to upstream this because those tests weren't triggered on the change due to a bug:
web-platform-tests#37623

There was an attempted fix for this:
web-platform-tests#37549

But, quoting jgraham from the WPT Matrix chat:

> the actual fix failed a test I wrote and now I need to spend some more time investigating

Original change's description:
> WPT: Allow `window.onload` to contain multiple `test()`s
>
> ******************************************************************
> *** SHERIFFS: please don't revert this CL if it causes web_tests
>               to flake/fail. If that happens, the cause is a bad
>               test. Please mark that test as flaky/fail in
>               TestExpectations, with a new crbug. Please block the
>               new bug against crbug.com/1395228. Thanks!
> ******************************************************************
>
> Prior to this CL, a test like this:
>
> ```
> <script>
> window.onload = () => {
>   test((t) => { ... }, 'test 1');
>   test((t) => { ... }, 'test 2');
>   test((t) => { ... }, 'test 3');
> };
> </script>
> ```
>
> would not run anything after test #1. The issue is that the testharness
> immediately adds a window load handler that marks `all_loaded = true`,
> and that ends the tests as soon as the first result from the first test
> is processed. (The test runner waits for the first test because
> `Tests.prototype.all_done()` also waits until `this.tests.length > 0`.)
> There were various mitigating corner cases, such as if you started
> the list of tests with a promise_test(), that would increment a
> counter that kept the rest of the tests alive. Etc.
>
> With this CL, the testharness-added window.onload handler runs a
> setTimeout(0), so that `all_loaded` is only set to true after all of
> the tests are loaded by any window.onload handler.
>
> This exposed a few tests that should have been failing but were
> masked by the lack of test coverage - bugs have been filed for
> those. Also, several tests that were working around this via various
> means are also cleaned up in this CL. I'm sure there are more of
> those.
>
> Bug: 1395228,1395226,1307772
> Change-Id: I6f12b5922186af4e1e06808ad23b47ceac68559c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4074305
> Reviewed-by: Weizhong Xia <weizhong@google.com>
> Auto-Submit: Mason Freed <masonf@chromium.org>
> Reviewed-by: Mason Freed <masonf@chromium.org>
> Commit-Queue: Mason Freed <masonf@chromium.org>
> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1081558}

Bug: 1395228,1395226,1307772
Change-Id: Icbddad3a8bb47473bcbc331f424661b9041addf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111318
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085925}
foolip pushed a commit that referenced this pull request Feb 7, 2023
In the case that a popover contains an invoker that points back to that
invoker, the tab navigation code used to get confused. E.g.:

```
<div id="menu" popover>
  <button autofocus popoverhidetarget="menu">Button #1</button>
  <button popoverhidetarget="menu">Button #2</button>
</div>
```

In this case, trying to tab between the first and second button would
break because the second button appeared to be an invoker for a new
popover, when in reality it was an invoker for the same popover.

Fixed: 1399601
Bug: 1307772
Change-Id: I276370d7c8eee0dd32f0c89da202a0d3777bf911
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4133482
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1089080}
foolip pushed a commit that referenced this pull request Oct 6, 2023
…each time of the loop

There are 2 possible scenarios which are not handled by the method.

1. Moving content node to new `<blockquote>` has already been moved to outside
of the editing host.
2. There is no container to insert new `<blockquote>`, e.g., in an inline
editing host.

In the case #1, we should ignore the ex-child node.  In the case #2, we should
abort it.  Note that Chrome inserts `<blockquote>` even if there is no proper
container.  However, such behavior is disagreed in interop-2023.  Therefore,
it's okay just to abort it for now.

Depends on D180781

Differential Revision: https://phabricator.services.mozilla.com/D180782

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1756237
gecko-commit: 42f3f3ab11b47f1d56d8bcd6a128398539dd1f23
gecko-reviewers: m_kato
foolip pushed a commit that referenced this pull request Oct 6, 2023
…eb-platform-tests#40504)

* [wdspec] browsingContext.print: fix rounding error in page.py test

[pytest](https://github.com/web-platform-tests/wpt/blob/7a087d54be8b6c0ca0181a86dc1ff0b28461c383/webdriver/tests/support/image.py)
uses:

    def cm_to_px(cm): return round(cm * 96 / 2.54)

[js](https://github.com/web-platform-tests/wpt/blob/7a087d54be8b6c0ca0181a86dc1ff0b28461c383/tools/wptrunner/wptrunner/print_pdf_runner.html)
uses:

    const viewport = page.getViewport({ scale: 96. / 72. });
    ...
    canvas.height = viewport.height;
    canvas.width = viewport.width;

This produces a rounding error, even though the dimension is correct:

    >       assert cm_to_px(expected_dimensions["height"]) == height
    E       assert 454 == 453
    E         +454
    E         -453

The inconsistency of rounding in both ends becomes clear when we
eliminate "round" in the pytest side:

    >       assert cm_to_px(expected_dimensions["height"]) == height
    E       assert 453.54330708661416 == 453
    E         +453.54330708661416
    E         -453

There are multiple ways to fix this issue.

Option #1: Use "floor" instead of "round" in pytest.

Option #2: Use a range in the assertion comparison, allowing a
difference of up to +-1.0. This is what this PR does.

The comparison is performed in
[`assert_pdf_dimensions`](https://github.com/web-platform-tests/wpt/blob/b6107cc1ac8b9c2800b4c8e58af719b8e4d9b8db/webdriver/tests/support/fixtures_bidi.py#L210).

The problematic part is .96 / .72 which evaluates to 4/3 = 1.333333....

* use floor in cm_to_px instead of round

* compare to floor and to round instead of a range

* Revert "compare to floor and to round instead of a range"

This reverts commit 63f894e.

* Revert "use floor in cm_to_px instead of round"

This reverts commit 7e65d91.
foolip pushed a commit that referenced this pull request Oct 6, 2023
Because of the order of operations for Clone(), previous to this CL,
the typical sequence would be:

 1. Clone the element
 2. Clone the children of the element (recursing to step #1).
 3. AppendChild() each cloned child to its parent cloned element.
 4. (in the caller of Clone) AppendChild the cloned element to its
    eventual parent.

Because each AppendChild triggers a call to Node::InsertedInto() for
*all descendants of the appended element* [1], the fact that the tree
is constructed bottom-up (leaf nodes first) means that InsertedInto()
is called N^2 times, where N is the depth of the cloned tree.

Because clone-and-append is a very common pattern, this CL adds an
`append_to` argument to `Clone()`, which appends to the parent before
appending the children.

This CL also adds a perf test for this scenario (cloning a deep tree).
Locally, on a debug build, this test gives 0.13 runs/s before this CL,
and 0.40 runs/s after, for a 3.1X speedup.

[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/container_node.cc;l=1006;drc=5d60017dba57e86d477634812e1340127734f8a7

Bug: 1453291
Change-Id: Icdd75c45aa5ecc4fe8bb5d1ff0b7a2b27bec2171
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4728983
Reviewed-by: David Baron <dbaron@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1177922}
foolip pushed a commit that referenced this pull request Apr 24, 2024
…rners

We had two issues:
1.  Before we had fast rounded corners, we always created mask layers
for rounded corner clips, and the mask layer made the scroll begin
unreliable and fall back to the main thread. With fast rounded corners,
the scrolls were treated as reliable without checking if the point is
in or out of the rounded corners.
2. If the scroller has a rounded corner by itself (instead of from an
ancestor), as we only create InnerBorderRadiusClip for the contents,
the compositor doesn't actually know which part of the layer bounds
is transparent to hit test (e.g. if the scroller has a border which
is outside of the InnerBorderRadiusClip). Now with HitTestOpaqueness,
such layers have HitTestOpaqueness::kMixed.

This CL changes the behavior of
LayerTreeImpl::FindLayersUpToFirstOpaqueToHitTest (renamed from
FindLayerUpToFirstScrollableOrOpaqueToHitTest):
- For issue #1: LayerImpl::OpaqueToHitTest() also checks whether the
  layer is affected by any fast rounded corners;
- For issue #2: FindLayerUpToFirstOpaqueToHitTest checks only
  OpaqueToHitTest() (without checking IsScrollerOrScrollbar())
  because a hit test on a scrollable layer is reliable only if it's
  opaque to hit test.

Bug: 40277896
Change-Id: I1acb16f2c6790760661e8239ea1599035f83ea51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5466909
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1291538}
foolip pushed a commit that referenced this pull request May 14, 2024
Bug #1: AbstractRange::(Mark|Unmark)Descendants should always use
the shadow tree of web-exposed shadow root, instead of using
light DOM elements of the host.

Bug #2: aRange could possibly create mCrossShadowBoundaryRange
first (due to boundaries are in different tree), and later
moves the boundaries to the same tree. When this happens, we
should remove mCrossShadowBoundaryRange and use the default
range to represent it.

Differential Revision: https://phabricator.services.mozilla.com/D207608

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1891783
gecko-commit: 0f54a84c32d1c22d71ff7307944b824639adbd6f
gecko-reviewers: jjaschke, smaug, dom-core
foolip pushed a commit that referenced this pull request May 16, 2024
Since @page border box layout objects aren't in the the layout tree, any
code that wants to walk up the tree to find the containing block will be
in for a surprise.

This would happen if percentage-based @page padding was used [1].
Recomputing padding during painting when we have already done it during
layout is rather pointless anyway. Read it out directly from the
fragment.

[1] #1 blink::LayoutBox::ContainingBlockLogicalWidthForContent()
    #2 blink::LayoutBoxModelObject::ComputedCSSPadding()
    #3 blink::LayoutBoxModelObject::PaddingTop()
    #4 blink::LayoutBoxModelObject::PaddingOutsets()
    #5 blink::BoxPainterBase::PaintFillLayer()
    #6 blink::BoxPainterBase::PaintFillLayers()
    #7 blink::BoxFragmentPainter::PaintBackground()

Bug: 40286153
Change-Id: I1e6e92c2ce1d81aab2673ec9a877eac455534102
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5526469
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1300711}
foolip pushed a commit that referenced this pull request Jul 2, 2024
WebRTC is one form of network communication that should
be disabled when window.fence.disableUntrustedNetwork is called
in a fenced frame. However,

1. We don't have any identified use cases for WebRTC in fenced frames
2. The revocation process would be more involved than other forms of
network access, which would provide little benefit per #1.
3. Entirely disabling WebRTC PeerConnection instead is beneficial for privacy and does not break existing fenced frame use cases.

This CL disables RTCPeerConnection construction entirely in fenced
frames, regardless of whether window.fence.disableUntrustedNetwork
was called or not. The change is behind an existing flag so that
it does not ship until other forms of network revocation do.

Disabling RTCPeerConnection *can* be handled entirely by the renderer,
but a compromised renderer could potentially circumvent this to
construct a peer connection anyway. A follow-up CL will add
a browser-side control to ensure that this does not occur.

Change-Id: Iaa2caaddeee70852179332dd89c5dbbac3ffcfbf
Bug: 41488151
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5527514
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Andrew Verge <averge@chromium.org>
Reviewed-by: Liam Brady <lbrady@google.com>
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1319162}
foolip pushed a commit that referenced this pull request Mar 31, 2025
Remove AILanguageDetectorFactory and expose create and availability
statically on LanguageDetector.

This is a followup change to CLs crrev.com/c/6402873,
crrev.com/c/6402775.

Note: This change is a cherry-pick of crrev.com/c/6397965.
Some changes have been made as part of rebasing + fixing previous
CQ test failures. PS #1 represents the cherry-picked change prior
to rebase + minor edits.

Fixed: 402165734
Change-Id: Id384ebe31ccfe9e0efd65dd7c890cd66875a9ed8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6403274
Reviewed-by: Mike Wasserman <msw@chromium.org>
Commit-Queue: Christine Hollingsworth <christinesm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1439000}
foolip pushed a commit that referenced this pull request Apr 28, 2025
This is a preparation change to the upcoming change that disallows
non-trustworthy plaintext HTTP prerendering.

This CL now runs external/wpt/speculation-rules/ wpts over HTTPs.

Bug: 340895233
Change-Id: Id820f192f5e973245c2dbbf1ca8daa5078abad78
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6398184
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Fabio Rocha <fabio.rocha@microsoft.com>
Commit-Queue: Jessica Chen <peiche@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1450755}
foolip pushed a commit that referenced this pull request Oct 15, 2025
…& compile_module_script to fix the inline script reporting wrong line issue (web-platform-tests#39415)

Originally, the function compile_module_script hardwires the value 1 when invoking CompileOptionsWrapper::new(). This is fine if the script is written in separate JS file, but for inline scripts, it will cause confusion if the <script> tag doesn't start from line #1.

Credits to JDM for actually pointing out which functions to fix.

Testing: There are WPT tests for this change, specifically: tests/wpt/tests/html/semantics/scripting-1/the-script-element/module/evaluation-error-5.html
Signed-off-by: RichardTjokroutomo <richard.tjokro2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant