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

Issue #2 - Updated diagram with latest methods #143

Closed
wants to merge 1 commit into from

Conversation

steelejoe
Copy link
Contributor

Updating stack-overview diagram to reflect latest methods

@ddorwin
Copy link
Contributor

ddorwin commented Jan 19, 2016

Thanks. LG except for some visual issues and nits. Please also change the commit message to be "Fix #2: Update the figure" or something like that. The important part is "Fix #2:"

Unless otherwise noted, these issues appear in both Chrome and Firefox.

  • In Firefox, text is no longer centered. Examples: CDN, Net, MediaKeySession, CDM. Also, Media Stack runs into the first MediaKeySession.
  • The first MediaKeySession is not centered even in Chrome. In Firefox, it extends outside its box.
  • The font changed. The main problem is that the italics text looks bad, especially the e's.
  • The MediaKeySession boxes appear to be at an angle. When scaled up, the horizontal lines are not straight.
  • The optional (right) update arrow still has a weird double head.
  • Please make the heads of the left update() arrow and the top (up) Frame arrow touch their target objects.

@ddorwin
Copy link
Contributor

ddorwin commented Jan 19, 2016

  • The MediaKeySession boxes are different sizes. I think the right one can be trimmed on the right.

@steelejoe
Copy link
Contributor Author

I think this has everything you wanted. You are correct the fonts changed, because the original SVG (as far as I can tell) used outlining which looks nice but does not preserve font information. So I made a best guess. I believe the wonky arrows heads should all be corrected as well.

@steelejoe steelejoe changed the title Fix for EME issue 2 Fix #2 - Updated diagram with latest methods Feb 5, 2016
@steelejoe steelejoe changed the title Fix #2 - Updated diagram with latest methods Issue #2 - Updated diagram with latest methods Feb 5, 2016
@ddorwin
Copy link
Contributor

ddorwin commented Feb 16, 2016

LGTM. Thanks!

Is there a reason the title was changed back? Perhaps when you force pushed the new commit, replacing the previous one?

There are a couple minor issues, but we can fix those in a separate PR if you want:

  • The MediaKeySession boxes appear to be at an angle. When scaled up, the horizontal lines are not straight. This is more pronounced in Chrome that in Firefox.
  • The Frame up arrow appears to have a double head.

@steelejoe
Copy link
Contributor Author

The title change was likely the force commit. I am not seeing the slant you are seeing on Chrome or Firefox, but I will test on other platforms. Let me put together another PR.

@ddorwin
Copy link
Contributor

ddorwin commented Feb 18, 2016

I can't perceive a slant, but the horizontal lines are not solid as if they are at an angle. Like ~~~----___ but more subtle. This is more obvious at high resolutions.

@ddorwin
Copy link
Contributor

ddorwin commented Mar 7, 2016

@steelejoe, what do you want to do? Shall we commit this and continue to fix minor details later?

@ddorwin
Copy link
Contributor

ddorwin commented Mar 24, 2016

@steelejoe, should we land this and make improvements later?

@steelejoe
Copy link
Contributor Author

Sorry was looking into the wrong place for updates. Yes please. Let's
commit this and I will work on correcting the slant after.
On Mar 24, 2016 10:41 AM, "ddorwin" notifications@github.com wrote:

@steelejoe https://github.com/steelejoe, should we land this and make
improvements later?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#143 (comment)

@ddorwin
Copy link
Contributor

ddorwin commented Mar 28, 2016

Merged as 336e7bd. Thank you!

@ddorwin ddorwin closed this Mar 28, 2016
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.

2 participants