Skip to content

Add comments, rename things, remove unused code #3143

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

Merged
merged 10 commits into from
Oct 2, 2018

Conversation

bholley
Copy link
Contributor

@bholley bholley commented Sep 29, 2018

CC @nical @kvark @gw3583

r? whoever wants to take it.


This change is Reviewable

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good! I have a few minor concerns, please feel free to r=me anyhow.

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @bholley)


webrender/src/internal_types.rs, line 48 at r1 (raw file):

}

/// Represents a texture that serves as input to a shader.

I think the meaning of this enum is more about where the texture comes from, rather than where it goes to (i.e. the shader).


webrender/src/render_task.rs, line 190 at r1 (raw file):

    ///
    /// The second member specifies the width and height of the task
    /// output, and the first member is initially left blank. During the build

"blank" -> None ?


webrender/src/tiling.rs, line 342 at r1 (raw file):

pub struct GlyphJob;

/// Represents a regular color output surface, RGBA32.

In WR, the convention is to specify bits per channel here, i.e. RGBA8


webrender/src/tiling.rs, line 342 at r1 (raw file):

pub struct GlyphJob;

/// Represents a regular color output surface, RGBA32.

I'd say, rather, that this structure contains all the work (in the form of instance arrays) needed to fill a color surface

Copy link
Contributor Author

@bholley bholley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kvark)


webrender/src/internal_types.rs, line 48 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think the meaning of this enum is more about where the texture comes from, rather than where it goes to (i.e. the shader).

Done.


webrender/src/render_task.rs, line 190 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

"blank" -> None ?

Done.


webrender/src/tiling.rs, line 342 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

In WR, the convention is to specify bits per channel here, i.e. RGBA8

I was cribbing from the existing comment in RenderTargetKind, I'll fix that one up as well.


webrender/src/tiling.rs, line 342 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I'd say, rather, that this structure contains all the work (in the form of instance arrays) needed to fill a color surface

Got it. I assume I should fix up AlphaRenderTarget as well.

@bholley bholley changed the title Add comments around RenderTargets and a few related things Add comments, rename things, remove unused code Oct 2, 2018
@bholley
Copy link
Contributor Author

bholley commented Oct 2, 2018

Added more commits. r? @kvark

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong:

Reviewed 28 of 28 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@kvark
Copy link
Member

kvark commented Oct 2, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit f29f465 has been approved by kvark

@bors-servo
Copy link
Contributor

⌛ Testing commit f29f465 with merge 005b36d...

bors-servo pushed a commit that referenced this pull request Oct 2, 2018
Add comments, rename things, remove unused code

CC @nical @kvark @gw3583

r? whoever wants to take it.

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

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 005b36d to master...

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.

3 participants