Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

./mach run --browserhtml on Windows #10943

Closed
wants to merge 1 commit into from
Closed

Conversation

@adamncasey
Copy link
Contributor

adamncasey commented Apr 30, 2016

This adds some windows specific behaviour to the ./mach run --browserhtml command:

Remove the webrender option
Convert the browserhtml path into a relative path to avoid a mingw->windows path conversion.

@larsbergstrom said that webrender support will come to windows ( #10243 (comment) ), which will remove the need for part of this soon. But it makes testing slightly nicer.


This change is Reviewable

@highfive
Copy link

highfive commented Apr 30, 2016

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/servo/command_base.py, python/servo/post_build_commands.py
@adamncasey
Copy link
Contributor Author

adamncasey commented Apr 30, 2016

I'm not totally convinced by converting it to a relative path. But it happens to work around the problem of msys python generating a unix format path, and servo expecting paths in windows' format.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Apr 30, 2016

@bors-servo r+

I'm approving this so that it can be tested, but I do not expect browser.html to work at all well with the default CPU renderer. Like most other browser engines, it does quite poorly on such a complicated page.

@bors-servo
Copy link
Contributor

bors-servo commented Apr 30, 2016

📌 Commit 738ae67 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Apr 30, 2016

Testing commit 738ae67 with merge f935a14...

bors-servo added a commit that referenced this pull request Apr 30, 2016
./mach run --browserhtml on Windows

This adds some windows specific behaviour to the ./mach run --browserhtml command:

Remove the webrender option
Convert the browserhtml path into a relative path to avoid a mingw->windows path conversion.

@larsbergstrom said that webrender support will come to windows ( #10243 (comment) ), which will remove the need for part of this soon. But it makes testing slightly nicer.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10943)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 30, 2016

💔 Test failed - mac-rel-css

@adamncasey
Copy link
Contributor Author

adamncasey commented Apr 30, 2016

I'll fix the tidy problems & push a new commit soon

@UK992
Copy link
Contributor

UK992 commented Apr 30, 2016

With servo/webrender#247 made Webrender actually work well on Windows. I've been testing it for a month now.

EDIT: Browser.html, via --browserhtml open blank window, when directly via path from same source with -w it works fine.

@UK992
Copy link
Contributor

UK992 commented May 1, 2016

Now that Webrender PR for Windows was merged, may i suggest to exclude command -b instead, because window without native titlebar is currently useless on Windows.

@adamncasey
Copy link
Contributor Author

adamncasey commented May 1, 2016

That seems sensible

@adamncasey
Copy link
Contributor Author

adamncasey commented May 1, 2016

Does disabling the titlebar work on Linux? It might make more sense to only pass -b if it's running on a mac

@paulrouget
Copy link
Contributor

paulrouget commented May 2, 2016

Does disabling the titlebar work on Linux?

No.

Pass browserhtml path as relative path to avoid a mingw->windows path conversion
@highfive
Copy link

highfive commented May 4, 2016

New code was committed to pull request.

@adamncasey
Copy link
Contributor Author

adamncasey commented May 4, 2016

I don't have an appropriate box to test this on, but borderless should now only enable on OSX.

When the dependency on webrender is updated past servo/webrender@672a360 --browserhtml will also work on Windows

@jasonwilliams
Copy link
Contributor

jasonwilliams commented May 5, 2016

@bors-servo
Copy link
Contributor

bors-servo commented May 23, 2016

The latest upstream changes (presumably #11332) made this pull request unmergeable. Please resolve the merge conflicts.

@metajack
Copy link
Contributor

metajack commented May 25, 2016

Windows webrender seems to work fine on master. What am I missing?

Previously, bors-servo wrote…

The latest upstream changes (presumably #11332) made this pull request unmergeable. Please resolve the merge conflicts.


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


python/servo/command_base.py, line 110 [r1] (raw file):

    if sys.platform == 'win32':
        return True
    if platform.system() == 'Windows':

I think this can be written simply as:

return sys.platform == 'win32' or sys.platform = 'msys'


Comments from Reviewable

@larsbergstrom
Copy link
Contributor

larsbergstrom commented May 27, 2016

I don't think we need this anymore, now that WebRender on master works on Windows. Hooray!

@UK992
Copy link
Contributor

UK992 commented May 27, 2016

@larsbergstrom Webrender is solved, but latest commit disable useless borderless window and fix browser.html path related issue (described above).

@UK992 UK992 mentioned this pull request Jun 1, 2016
bors-servo added a commit that referenced this pull request Jun 9, 2016
Fix ./mach --browserhtml

Rebased #10943 and simplified by #10943 (comment).
Fixes path related issue with browser.html on Windows and enable borderless window only on OSX.

r? @metajack

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11543)
<!-- Reviewable:end -->
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…l); r=metajack

Rebased servo/servo#10943 and simplified by servo/servo#10943 (comment).
Fixes path related issue with browser.html on Windows and enable borderless window only on OSX.

r? metajack

Source-Repo: https://github.com/servo/servo
Source-Revision: 28c0b869cab72020edbc54e5ccd761dc2dbb92b2

UltraBlame original commit: 3b982de13bc0b3ae90d52eba9977d10c33266c9a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…l); r=metajack

Rebased servo/servo#10943 and simplified by servo/servo#10943 (comment).
Fixes path related issue with browser.html on Windows and enable borderless window only on OSX.

r? metajack

Source-Repo: https://github.com/servo/servo
Source-Revision: 28c0b869cab72020edbc54e5ccd761dc2dbb92b2

UltraBlame original commit: 3b982de13bc0b3ae90d52eba9977d10c33266c9a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…l); r=metajack

Rebased servo/servo#10943 and simplified by servo/servo#10943 (comment).
Fixes path related issue with browser.html on Windows and enable borderless window only on OSX.

r? metajack

Source-Repo: https://github.com/servo/servo
Source-Revision: 28c0b869cab72020edbc54e5ccd761dc2dbb92b2

UltraBlame original commit: 3b982de13bc0b3ae90d52eba9977d10c33266c9a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.